On Tue, Dec 19, 2023 at 11:46 PM Vladimir Oltean olteanv@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