Messages in this thread | | | Date | Tue, 15 Mar 2022 21:49:01 -0600 | Subject | Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv() | From | David Ahern <> |
| |
On 3/15/22 9:08 PM, Jakub Kicinski wrote: > On Mon, 14 Mar 2022 21:33:10 +0800 menglong8.dong@gmail.com wrote: >> + reason = SKB_DROP_REASON_NOT_SPECIFIED; >> if (!pskb_may_pull(skb, 12)) >> goto drop; > > REASON_HDR_TRUNC ? > >> ver = skb->data[1]&0x7f; >> - if (ver >= GREPROTO_MAX) >> + if (ver >= GREPROTO_MAX) { >> + reason = SKB_DROP_REASON_GRE_VERSION; > > TBH I'm still not sure what level of granularity we should be shooting > for with the reasons. I'd throw all unexpected header values into one > bucket, not go for a reason per field, per protocol. But as I'm said > I'm not sure myself, so we can keep what you have..
I have stated before I do not believe every single drop point in the kernel needs a unique reason code. This is overkill. The reason augments information we already have -- the IP from kfree_skb tracepoint.
> >> goto drop; >> + } >> >> rcu_read_lock(); >> proto = rcu_dereference(gre_proto[ver]); >> - if (!proto || !proto->handler) >> + if (!proto || !proto->handler) { >> + reason = SKB_DROP_REASON_GRE_NOHANDLER; > > I think the ->handler check is defensive programming, there's no > protocol upstream which would leave handler NULL. > > This is akin to SKB_DROP_REASON_PTYPE_ABSENT, we can reuse that or add > a new reason, but I'd think the phrasing should be kept similar. > >> goto drop_unlock; >> + } >> ret = proto->handler(skb);
| |