netdev - Re: [PATCH 1/7] net: Don't set transport offset to invalid value

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: <CAF=yD-+YRkKBN5ALc49hwmeyWTEwd=1ykCHvqyOuHznJL6ZqEw@mail.gmail.com>
Date:   Thu, 24 Jan 2019 09:19:25 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Maxim Mikityanskiy <maximmi@...lanox.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Jason Wang <jasowang@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: Re: [PATCH 1/7] net: Don't set transport offset to invalid value

On Thu, Jan 24, 2019 at 4:48 AM Maxim Mikityanskiy <maximmi@...lanox.com> wrote:
>
> > > but the general idea is that we
> > > report this status, so if you say that my version is also good for you,
> > > I'll leave it as is. It was just a rationale for my decision.
> >
> > It's fine. But please avoid the code churn in xenvif_tx_submit
> > with to extra indentation. This is equivalent:
> >
> > -               if (skb_is_gso(skb)) {
> > +               if (skb_is_gso(skb) && th_set) {
> >
> > More fundamentally, the code has the assumption that th_set
> > always holds if skb_is_gso(skb). Why add the check? This is
> > another example that the return value is not really needed.
>
> What about
>
> if (skb_is_gso(skb)) {
>         BUG_ON(!skb_transport_header_was_set(skb));
>
> ?

Good idea to validate. Please on error use WARN_ON_ONCE and free the
packet as with other errors in this function. BUG_ONs are problematic
when a path to them is found.

The other option is to stay as close to the current logic as possible
in this driver and set the mac header at offset 0 if probe fails.
Either through the return value or skb_transport_header_was_set.

> I think it's cleaner than skipping the action here if dissect failed and
> propagating a potential bug further.
>
> > > > If this is the only reason for the boolean return value, using
> > > > skb_transport_header_was_set() is more standard (I immediately know
> > > > what's happening when I read it), slightly less code change and avoids
> > > > introducing a situation where the majority of callers ignore a return
> > > > value. I think it's preferable. But these merits are certainly
> > > > debatable, so either is fine.
> > >
> > > From my side, I wanted to avoid calling skb_transport_header_was_set
> > > twice,  so I made skb_try_probe_transport_header return whether it
> > > succeeded or not. I think "try" in the function name indicates this idea
> > > pretty clearly. This result status is pretty useful, it just happened
> > > that it's not needed in many places,
> >
> > Which is an indication that it's perhaps not needed.
>
> Well, from the point of view of the function, it looks reasonable to
> notify the caller whether the call was successful or not... You know,
> many functions return error codes.
>
> However, this case is rather special, because it turned out that we
> don't actually need that error status immediately, but it can be
> requested much later, and we already have skb_transport_header_was_set
> for that.
>
> So, considering these points, the return value can be removed, as all
> use cases are "probe now, check later", not "probe now and handle errors
> immediately".