[PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Linus Walleij]

On Tue, Dec 19, 2023 at 11:46 PM Vladimir Oltean <olteanv at gmail.com> wrote:
> On Tue, Dec 19, 2023 at 05:29:32PM +0100, Maxime Chevallier wrote:

> > > I can certainly add a helper such as skb_eth_raw_ethertype()
> > > to <linux/if_ether.h> that will inspect the actual ethertype in
> > > skb->data.
> > >
> > > It's the most straight-forward approach.
> >
> > Agreed :)
>
> If you rewrite that patch to use skb_vlan_eth_hdr() to get a struct
> vlan_ethhdr pointer through which h_vlan_proto and h_vlan_encapsulated_proto
> are accessible, I don't see much value in writing that helper. It is
> going to beg the question how generic should it be - should it also
> treat ETH_P_8021AD, should it treat nested VLANs?

I guess I should just post the patches inline. (It came from both
Erics and Maximes suggestion really.)

Actually I wrote two helpers, one to get the ethertype from the
ethernet frame which is pretty straight-forward.

include/linux/if_ether.h

+/* This determines the ethertype incoded into the skb data without
+ * relying on skb->protocol which is not always identical.
+ */
+static inline u16 skb_eth_raw_ethertype(const struct sk_buff *skb)
+{
+       struct ethhdr *hdr;
+
+       /* If we can't extract a header, return invalid type */
+       if (!skb_pointer_if_linear(skb, 0, ETH_HLEN))
+               return 0x0000U;
+
+       hdr = skb_eth_hdr(skb);
+
+       return ntohs(hdr->h_proto);
+}

Then for *this* driver I need to check for the ethertype
ETH_P_8021Q what is inside it, one level down, and that is a
separate helper. And I named it skb_vlan_raw_inner_ethertype()
It will retrieve the inner type no matter

include/linux/if_vlan.h

+/* This determines the inner ethertype incoded into the skb data without
+ * relying on skb->protocol which is not always identical.
+ */
+static inline u16 skb_vlan_raw_inner_ethertype(const struct sk_buff *skb)
+{
+       struct vlan_ethhdr *vhdr;
+
+       if (!skb_pointer_if_linear(skb, 0, VLAN_ETH_HLEN))
+               return 0x0000U;
+
+       vhdr = vlan_eth_hdr(skb);
+       return ntohs(vhdr->h_vlan_encapsulated_proto);
+}

(We can bikeshed the name of the function. *_inner_protocol maybe.)

It does not handle nested VLANs and I don't see why it should since
the immediate siblings in if_vlan.h does not, i.e.
vlan_eth_hdr(), skb_vlan_eth_hdr(). It's pretty clear these helpers
all go just one level down. (We can add a *_descend_*()
helper the day someone needs that.)

> At the end of the day, you are trying to cover in software the cases for
> which the hardware engine can perform TX checksum offloading. That is
> going to be hardware specific.

Yeps and I am happy to fold these helpers inside of my driver if
they are not helpful to anyone else, or if that is the best idea for something
intended for a fix, i.e. an -rc kernel.

> I guess we should first try to answer the questions "what does
> skb->protocol represent?" and "does DSA use it correctly?" before
> even thinking about adding yet another fuzzy layer on top it.

Fair point! Let's take a step back. The kerneldoc says:

 *      @protocol: Packet protocol from driver

That's a bit vague and it was in the first commit in git history :/

But Eric probably knows the right way to use protocol.

But we know for sure that VLAN uses this for the outermost protocol
ETH_P_8021Q (etc).

I wonder how the network stack reacts if we set the skb->protocol
to whatever DSA taggers put at the position of the ethertype.

For RTL taggers probably this works because they use an elaborate
custom ethertype, but e.g. net/dsa/tag_mtk.c will just put in
"ethertype" 0x0000, 0x0001 or 0x0002, the two latter which are
formally ETH_P_802_3 and ETH_P_AX25 which I think is maybe
not so good to put into skb->protocol.

Another option is to set it to the ETH_P_DSA ethertype, currently
unused in the kernel.

Now this kind of thinking makes me insecure because:
git grep '\->protocol' net/

There is just sooooo much code inspecting ->protocol in the generic
network stack that this seems like inviting disaster.

Yours,
Linus Walleij