Hi Willem,
On Mon, Jun 10, 2024 at 4:21 AM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Chengen Du wrote:
Hi,
I would like to provide some additional explanations about the patch.
On Sat, Jun 8, 2024 at 10:54 AM Chengen Du chengen.du@canonical.com wrote:
The issue initially stems from libpcap. The ethertype will be overwritten as the VLAN TPID if the network interface lacks hardware VLAN offloading. In the outbound packet path, if hardware VLAN offloading is unavailable, the VLAN tag is inserted into the payload but then cleared from the sk_buff struct. Consequently, this can lead to a false negative when checking for the presence of a VLAN tag, causing the packet sniffing outcome to lack VLAN tag information (i.e., TCI-TPID). As a result, the packet capturing tool may be unable to parse packets as expected.
The TCI-TPID is missing because the prb_fill_vlan_info() function does not modify the tp_vlan_tci/tp_vlan_tpid values, as the information is in the payload and not in the sk_buff struct. The skb_vlan_tag_present() function only checks vlan_all in the sk_buff struct. In cooked mode, the L2 header is stripped, preventing the packet capturing tool from determining the correct TCI-TPID value. Additionally, the protocol in SLL is incorrect, which means the packet capturing tool cannot parse the L3 header correctly.
Link: https://github.com/the-tcpdump-group/libpcap/issues/1105 Link: https://lore.kernel.org/netdev/20240520070348.26725-1-chengen.du@canonical.c... Fixes: 393e52e33c6c ("packet: deliver VLAN TCI to userspace") Cc: stable@vger.kernel.org Signed-off-by: Chengen Du chengen.du@canonical.com
net/packet/af_packet.c | 57 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 2 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index ea3ebc160e25..8cffbe1f912d 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -538,6 +538,43 @@ static void *packet_current_frame(struct packet_sock *po, return packet_lookup_frame(po, rb, rb->head, status); }
+static u16 vlan_get_tci(struct sk_buff *skb) +{
struct vlan_hdr vhdr, *vh;u8 *skb_orig_data = skb->data;int skb_orig_len = skb->len;skb_push(skb, skb->data - skb_mac_header(skb));vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);if (skb_orig_data != skb->data) {skb->data = skb_orig_data;skb->len = skb_orig_len;}The reason for not directly using skb_header_pointer(skb, skb_mac_header(skb) + ETH_HLEN, ...) to get the VLAN header is due to the check logic in skb_header_pointer. In the SOCK_DGRAM and PACKET_OUTGOING scenarios, the offset can be a negative number, which causes the check logic (i.e., likely(hlen - offset >= len)) in __skb_header_pointer() to not work as expected.
The calculation is still correct?
I think that this is not the first situation where negative offsets can be given to skb_header_pointer.
The check will pass even if the offset is negative, but I believe this may not be the right approach. In my humble opinion, the expected check should be similar to the skb_push check, which ensures that after moving forward by the offset bytes, skb->data remains larger than or equal to skb->head to avoid accessing out-of-bound data. It might be worth considering adding a check in __skb_header_pointer to handle negative offsets, as this seems logical. However, this change could impact a wider range of code. Please correct me if I am mistaken.
While it is possible to modify __skb_header_pointer() to handle cases where the offset is negative, this change could affect a wider range of code.