Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[Menglong Dong]

On Wed, Mar 16, 2022 at 11:49 AM David Ahern <dsahern@xxxxxxxxxx> wrote:
>
> On 3/15/22 9:08 PM, Jakub Kicinski wrote:
> > On Mon, 14 Mar 2022 21:33:10 +0800 menglong8.dong@xxxxxxxxx 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.

Is this reason unnecessary? I'm not sure if the GRE version problem should
be reported. With versions not supported by the kernel, it seems we
can't get the
drop reason from the packet data, as they are fine. And previous seems not
suitable here, as it is a L4 problem.

I'll remove the reason here if there is no positive reply.

Thanks!
Menglong Dong
>
> >
> >>              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);
>