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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Thu, Jan 24, 2019 at 4:48 AM Maxim Mikityanskiy <maximmi@mellanox.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".