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