netdev - Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADxym3bK09hm2zn8yRU5g9fm=MhN7j9xZAjJEoV6_wpuvs9o-w@mail.gmail.com>
Date:   Wed, 16 Mar 2022 12:53:40 +0800
From:   Menglong Dong <menglong8.dong@...il.com>
To:     David Ahern <dsahern@...nel.org>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>, xeb@...l.ru,
        David Miller <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Menglong Dong <imagedong@...cent.com>,
        Eric Dumazet <edumazet@...gle.com>, Martin Lau <kafai@...com>,
        Talal Ahmad <talalahmad@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Alexander Lobakin <alobakin@...me>,
        Hao Peng <flyingpeng@...cent.com>,
        Mengen Sun <mengensun@...cent.com>, dongli.zhang@...cle.com,
        LKML <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        Biao Jiang <benbjiang@...cent.com>
Subject: Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

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

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);
>