Re: [PATCH net-next 1/2] net/packet: Reduce VLAN tag code duplication

[Willem de Bruijn]

Joe Damato wrote:
> On Wed, Aug 12, 2026 at 08:28:39AM -0400, Willem de Bruijn wrote:
> > 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@xxxxxxx>
> > 
> > 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.
> 
> OK. In that case, I'll mark the series as rejected below. I was also working
> on a test for ORIGDEV because many, many years ago I got bit by unexpected
> results mixing packet sockets with bonded interfaces.
> 
> But, I'll refrain from touching anything related to af_packet including tests
> from now on.

It only applies to pure code cleanups.

> > > ---
> > >  net/packet/af_packet.c | 95 +++++++++++++++++++++++-------------------
> > >  1 file changed, 53 insertions(+), 42 deletions(-)

As said it is a bit subjective (I did add a Reviewed-by to patch 2/2).
And my opinion is just one. But this one to me makes the code less
obvious, and no shorter.

But, the real point remains that the bar for cleanups is high to
offset the dual risk of unintended side-effects, and especially
complicating (security) fix backports.

> > > 
> > > 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
> 
> [...]
> 
> > > @@ -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?
> 
> If you apply the patch to the tree and look at it, it'll be less confusing. It
> looks weird because of what git generated, but as far as my reading of the
> code goes, there is no behavior change.

I see what you mean, thanks.