linux-kernel - Re: [PATCH net-next 3/3] net: ipgre: 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: <20220316115027.7a153c4f@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 16 Mar 2022 11:50:27 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Menglong Dong <menglong8.dong@...il.com>
Cc:     David Ahern <dsahern@...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 3/3] net: ipgre: add skb drop reasons to
 gre_rcv()

On Wed, 16 Mar 2022 14:21:24 +0800 Menglong Dong wrote:
> > I feel like gre_parse_header() is a good candidate for converting
> > to return a reason instead of errno.
> 
> Enn...isn't gre_parse_header() returning the header length? And I
> didn't find much useful reason in this function.

Ah, you're right, it returns negative error or hdr_len.
We'd need to make the reason negative, I guess that's not pretty.

What made me wonder is that it already takes a boolean for csum error
and callers don't care _which_ error gets returned. 

We can replace the csum_err output param with reason code, then?

> > >               goto drop;
> > >
> > >       if (unlikely(tpi.proto == htons(ETH_P_ERSPAN) ||
> > > -                  tpi.proto == htons(ETH_P_ERSPAN2))) {
> > > -             if (erspan_rcv(skb, &tpi, hdr_len) == PACKET_RCVD)
> > > -                     return 0;
> > > -             goto out;
> > > -     }
> > > +                  tpi.proto == htons(ETH_P_ERSPAN2)))
> > > +             ret = erspan_rcv(skb, &tpi, hdr_len);
> > > +     else
> > > +             ret = ipgre_rcv(skb, &tpi, hdr_len);  
> >
> > ipgre_rcv() OTOH may be better off taking the reason as an output
> > argument. Assuming PACKET_REJECT means NOMEM is a little fragile.  
> 
> Yeah, it seems not friendly. I think it's ok to ignore such 'NOMEM' reasons?
> Therefore, we only need to consider the PACKET_NEXT return value, and
> keep ipgre_rcv() still.