Messages in this thread | | | From | Menglong Dong <> | Date | Wed, 16 Mar 2022 12:41:45 +0800 | Subject | Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv() |
| |
On Wed, Mar 16, 2022 at 11:08 AM Jakub Kicinski <kuba@kernel.org> 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 ?
I'm still not sure about such a 'pskb_pull' failure, whose reasons may be complex, such as no memory or packet length too small. I see somewhere return a '-NOMEM' when skb pull fails.
So maybe such cases can be ignored? In my opinion, not all skb drops need a reason.
> > > 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.. > > > 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.
With the handler not NULL, does it mean the gre version is not supported here, and this 'SKB_DROP_REASON_GRE_NOHANDLER' can be replaced with SKB_DROP_REASON_GRE_VERSION above?
> > > goto drop_unlock; > > + } > > ret = proto->handler(skb);
| |