On Tue, Dec 19, 2023 at 12:42 AM Linus Walleij [off-list ref] wrote:

On Mon, Dec 18, 2023 at 3:50 PM Eric Dumazet [off-list ref] wrote:

quoted

On Sat, Dec 16, 2023 at 8:36 PM Linus Walleij [off-list ref] wrote:

quoted

quoted

+       /* Dig out the the ethertype actually in the buffer and not what the
+        * protocol claims to be. This is the raw data that the checksumming
+        * offload engine will have to deal with.
+        */
+       p = (__be16 *)(skb->data + 2 * ETH_ALEN);
+       ethertype = ntohs(*p);
+       if (ethertype == ETH_P_8021Q) {
+               p += 2; /* +2 sizeof(__be16) */
+               ethertype = ntohs(*p);
+       }

Presumably all you need is to call vlan_get_protocol() ?

Sadly no. As the comment says: we want the ethertype that is actually in the
skb, not what is in skb->protocol, and the code in vlan_get_protocol() just
trusts skb->protocol to be the ethertype in the frame, especially if vlan
is not used.

This is often what we want: DSA switches will "wash" custom ethertypes
before they go out, but in this case the custom ethertype upsets the
ethernet checksum engine used as conduit interface toward the DSA
switch.

 Problem is that your code misses skb_header_pointer() or
pskb_may_pull() call...
Second "ethertype = ntohs(*p);" might access uninitialized data.

If this is a common operation, perhaps use a common helper from all drivers,
this would help code review a bit...