From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Joe Damato <joe@dama.to>,
netdev@vger.kernel.org,
Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>,
Paolo Abeni <pabeni@redhat.com>,
Simon Horman <horms@kernel.org>
Cc: andrew+netdev@lunn.ch, willemb@google.com,
Joe Damato <joe@dama.to>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 1/2] net/packet: Reduce VLAN tag code duplication
Date: Wed, 12 Aug 2026 08:28:39 -0400 [thread overview]
Message-ID: <willemdebruijn.kernel.57d27685522a@gmail.com> (raw)
In-Reply-To: <20260811184722.2612345-2-joe@dama.to>
Joe Damato wrote:
> Reduce code duplication for VLAN tag extraction by factoring the
> repeated code into a helper and using it.
>
> Signed-off-by: Joe Damato <joe@dama.to>
Especially with the improving AI bots, we're getting even more fixes
to PF_PACKET lately. Cleanup patches can block fix backports to stable.
The bar for pure cleanup patches has to be high to warrant that.
Plus, they add risk, if it is not trivial to review that they are NOOPs.
Subjective, but not sure this one warrants the cost.
> ---
> net/packet/af_packet.c | 95 +++++++++++++++++++++++-------------------
> 1 file changed, 53 insertions(+), 42 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 435756877aba..ee60dcc639ad 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -568,6 +568,27 @@ static u16 vlan_get_tci(const struct sk_buff *skb, struct net_device *dev)
> return ntohs(vh->h_vlan_TCI);
> }
>
> +static bool packet_get_vlan_tci_tpid(const struct sk_buff *skb,
> + struct net_device *dev,
> + u16 *tci, u16 *tpid)
> +{
> + if (skb_vlan_tag_present(skb)) {
> + *tci = skb_vlan_tag_get(skb);
> + *tpid = ntohs(skb->vlan_proto);
> + return true;
> + }
> +
> + if (unlikely(dev && eth_type_vlan(skb->protocol))) {
> + *tci = vlan_get_tci(skb, dev);
> + *tpid = ntohs(skb->protocol);
> + return true;
> + }
> +
> + *tci = 0;
> + *tpid = 0;
> + return false;
> +}
> +
> static __be16 vlan_get_protocol_dgram(const struct sk_buff *skb)
> {
> __be16 proto = skb->protocol;
> @@ -997,20 +1018,19 @@ static void prb_fill_vlan_info(struct tpacket_kbdq_core *pkc,
> struct tpacket3_hdr *ppd)
> {
> struct packet_sock *po = container_of(pkc, struct packet_sock, rx_ring.prb_bdqc);
> + struct net_device *dev = NULL;
> + u16 tci, tpid;
>
> - if (skb_vlan_tag_present(pkc->skb)) {
> - ppd->hv1.tp_vlan_tci = skb_vlan_tag_get(pkc->skb);
> - ppd->hv1.tp_vlan_tpid = ntohs(pkc->skb->vlan_proto);
> - ppd->tp_status = TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
> - } else if (unlikely(po->sk.sk_type == SOCK_DGRAM && eth_type_vlan(pkc->skb->protocol))) {
> - ppd->hv1.tp_vlan_tci = vlan_get_tci(pkc->skb, pkc->skb->dev);
> - ppd->hv1.tp_vlan_tpid = ntohs(pkc->skb->protocol);
> + if (po->sk.sk_type == SOCK_DGRAM)
> + dev = pkc->skb->dev;
> +
> + if (packet_get_vlan_tci_tpid(pkc->skb, dev, &tci, &tpid))
> ppd->tp_status = TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
Does this change behavior, now setting tp_status also for the second
branch, where previously the flags were not set?
> - } else {
> - ppd->hv1.tp_vlan_tci = 0;
> - ppd->hv1.tp_vlan_tpid = 0;
> + else
> ppd->tp_status = TP_STATUS_AVAILABLE;
> - }
> +
> + ppd->hv1.tp_vlan_tci = tci;
> + ppd->hv1.tp_vlan_tpid = tpid;
> }
>
> static void prb_run_all_ft_ops(struct tpacket_kbdq_core *pkc,
> @@ -2247,6 +2267,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> struct packet_type *pt, struct net_device *orig_dev)
> {
> enum skb_drop_reason drop_reason = SKB_CONSUMED;
> + struct net_device *vlan_dev = NULL;
> struct sock *sk = NULL;
> struct packet_sock *po;
> struct sockaddr_ll *sll;
> @@ -2262,6 +2283,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> __u32 ts_status;
> unsigned int slot_id = 0;
> int vnet_hdr_sz = 0;
> + u16 tci, tpid;
>
> /* struct tpacket{2,3}_hdr is aligned to a multiple of TPACKET_ALIGNMENT.
> * We may add members to them until current aligned size without forcing
> @@ -2436,18 +2458,12 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> h.h2->tp_net = netoff;
> h.h2->tp_sec = ts.tv_sec;
> h.h2->tp_nsec = ts.tv_nsec;
> - if (skb_vlan_tag_present(skb)) {
> - h.h2->tp_vlan_tci = skb_vlan_tag_get(skb);
> - h.h2->tp_vlan_tpid = ntohs(skb->vlan_proto);
> - status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
> - } else if (unlikely(sk->sk_type == SOCK_DGRAM && eth_type_vlan(skb->protocol))) {
> - h.h2->tp_vlan_tci = vlan_get_tci(skb, skb->dev);
> - h.h2->tp_vlan_tpid = ntohs(skb->protocol);
> + if (sk->sk_type == SOCK_DGRAM)
> + vlan_dev = skb->dev;
> + if (packet_get_vlan_tci_tpid(skb, vlan_dev, &tci, &tpid))
> status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
> - } else {
> - h.h2->tp_vlan_tci = 0;
> - h.h2->tp_vlan_tpid = 0;
> - }
> + h.h2->tp_vlan_tci = tci;
> + h.h2->tp_vlan_tpid = tpid;
> memset(h.h2->tp_padding, 0, sizeof(h.h2->tp_padding));
> hdrlen = sizeof(*h.h2);
> break;
> @@ -3547,7 +3563,9 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> }
>
> if (packet_sock_flag(pkt_sk(sk), PACKET_SOCK_AUXDATA)) {
> + struct net_device *vlan_dev = NULL;
> struct tpacket_auxdata aux;
> + u16 tci, tpid;
>
> aux.tp_status = TP_STATUS_USER;
> if (skb->ip_summed == CHECKSUM_PARTIAL)
> @@ -3562,29 +3580,22 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> aux.tp_snaplen = skb->len;
> aux.tp_mac = 0;
> aux.tp_net = skb_network_offset(skb);
> - if (skb_vlan_tag_present(skb)) {
> - aux.tp_vlan_tci = skb_vlan_tag_get(skb);
> - aux.tp_vlan_tpid = ntohs(skb->vlan_proto);
> - aux.tp_status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
> - } else if (unlikely(sock->type == SOCK_DGRAM && eth_type_vlan(skb->protocol))) {
> + rcu_read_lock();
> + if (unlikely(sock->type == SOCK_DGRAM &&
> + !skb_vlan_tag_present(skb) &&
> + eth_type_vlan(skb->protocol))) {
> struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll;
> - struct net_device *dev;
>
> - rcu_read_lock();
> - dev = dev_get_by_index_rcu(sock_net(sk), sll->sll_ifindex);
> - if (dev) {
> - aux.tp_vlan_tci = vlan_get_tci(skb, dev);
> - aux.tp_vlan_tpid = ntohs(skb->protocol);
> - aux.tp_status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
> - } else {
> - aux.tp_vlan_tci = 0;
> - aux.tp_vlan_tpid = 0;
> - }
> - rcu_read_unlock();
> - } else {
> - aux.tp_vlan_tci = 0;
> - aux.tp_vlan_tpid = 0;
> + vlan_dev = dev_get_by_index_rcu(sock_net(sk),
> + sll->sll_ifindex);
> }
> + if (packet_get_vlan_tci_tpid(skb, vlan_dev, &tci, &tpid))
> + aux.tp_status |= TP_STATUS_VLAN_VALID |
> + TP_STATUS_VLAN_TPID_VALID;
> + rcu_read_unlock();
> +
> + aux.tp_vlan_tci = tci;
> + aux.tp_vlan_tpid = tpid;
> put_cmsg(msg, SOL_PACKET, PACKET_AUXDATA, sizeof(aux), &aux);
> }
>
> --
> 2.53.0-Meta
>
next prev parent reply other threads:[~2026-08-12 12:28 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-08-11 18:47 [PATCH net-next 0/2] packet socket cleanup Joe Damato
2026-08-11 18:47 ` [PATCH net-next 1/2] net/packet: Reduce VLAN tag code duplication Joe Damato
2026-08-12 12:28 ` Willem de Bruijn [this message]
2026-08-12 18:24 ` Joe Damato
2026-08-11 18:47 ` [PATCH net-next 2/2] net/packet: Use copy_safe_from_sockptr to dedupe code Joe Damato
2026-08-12 12:19 ` Willem de Bruijn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=willemdebruijn.kernel.57d27685522a@gmail.com \
--to=willemdebruijn.kernel@gmail.com \
--cc=andrew+netdev@lunn.ch \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=horms@kernel.org \
--cc=joe@dama.to \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=willemb@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox