Re: [PATCH net v6] ip6_gre: use skb_vlan_inet_prepare() instead of pskb_inet_may_pull()

[Anton Danilov <littlesmilingcloud@gmail.com>]

Before I send a v7, I would like guidance on how much of this to fix in
net, because the one-line syzbot bug keeps pulling in pre-existing
problems on the ip6gre/ip6erspan transmit path, and each attempt to
cover them has drawn a new report.

The bug itself is narrow: for a VLAN-typed skb->protocol,
pskb_inet_may_pull() checks the length from skb->protocol while the code
that follows parses through the tags, so the inner header is read
without being pulled. Pulling it for that case closes the report.

What makes it larger is that the inner protocol is worked out three
times on the way out, each from a different base:

  - the length check, from skb->data;
  - the ipvX dispatch, from skb->mac_len via skb_protocol();
  - the inherited hop limit in ip6_tnl_xmit(), from the MAC header,
    after the GRE and ERSPAN headers are pushed (b09ab9c92e50).

As long as these three derive the protocol independently, a frame whose
bases disagree breaks one of the pairs: skb->data not at the MAC header
(a frame decoded by act_ife on egress), a tag not at the usual offset
(a DSA conduit's in-band tag), or no MAC header at all (L3 ip6gre). A
patch that lines up one pair leaves another exposed; the ERSPAN COS,
which a separate patch takes from the frame, is a fourth reader with its
own base.

The robust shape I see is to classify the payload once at
ndo_start_xmit and pass the result into ip6_tnl_xmit(), instead of
re-deriving it after the push. That removes the post-push walk and the
cross-file precondition behind it, but it changes the signature of the
exported ip6_tnl_xmit().

So, two questions:

  - for the net/stable fix of the syzbot report, would you prefer the
    minimal change (pull the inner header for a VLAN skb->protocol so
    the length check matches the existing parse, and leave the
    re-parse, the ERSPAN COS and the DSA/act_ife cases as separate
    net-next work), or the fuller rework now?
  - is changing the exported ip6_tnl_xmit() signature acceptable in
    net, or should anything touching it wait for net-next?

I have the measurements for the regression and for the pre-existing
reads and can post either form quickly once the scope is clear.

Thanks,
Anton Danilov

pw-bot: cr