Re: [PATCH net v2] net/packet: fix network header offset for non-VLAN raw packets

[Junnan Zhang <zhangjn_dev@163.com>]

Hi Willem,

Thanks for the detailed review.

> Where does it do this? validate_xmit_vlan does this on the physical
> device I think.

You're right. vlan_dev_hard_start_xmit() only attaches the tag metadata
via __vlan_hwaccel_put_tag(); the actual tag bytes are inserted later by
validate_xmit_vlan() on the physical device via
__vlan_hwaccel_push_inside(). I've corrected this in the v3 commit
message.

> That adds a bigger problem that the extra needed headroom of
> (hard_header_len - min_header_len) is not reserved at the start of
> the frame.

Understood. In this case it happens to work because
LL_RESERVED_SPACE_EX() rounds the reservation up, so enough headroom
remains in front of the MAC header for __vlan_insert_inner_tag() to
push the tag inside, and with the fix the MAC header correctly sits at
network_header - min_header_len. Agreed that disentangling the legacy
hard_header_len usage is beyond this bug fix targeting net; happy to
help test if you look at it for net-next.

> Instead of using min_header_len != hard_header_len to detect vlan
> devices, consider is_vlan_dev(). [...] So for now I would focus on
> the VLAN issue only.

Done in v3: the condition is now

	if (sock->type == SOCK_RAW && !is_vlan_packet && is_vlan_dev(skb->dev))

and I renamed is_vlan to is_vlan_packet to avoid confusion with
is_vlan_dev(). You're also right that the ARPHRD_ETHER check excluded
the other hard_header_len cases anyway, so I scoped the subject and
commit message back to VLAN subinterfaces.

> It sucks that we have to add another branch in the hot path for an
> edge case. Would be preferable if we can fix this in the vlan driver.
> But that will come too late for skb_probe_transport_header.

Agreed - the correction has to happen before skb_probe_transport_header()
in packet_snd(), so the VLAN driver xmit path is too late.

v3 sent as a new thread with a Link to v2.

Thanks,
Junnan