Re: [PATCH net v2 2/2] 8021q: delete cleared egress QoS mappings

[Paolo Abeni <pabeni@redhat.com>]

On 4/22/26 6:20 PM, Simon Horman wrote:
> On Mon, Apr 20, 2026 at 11:18:46AM +0800, Ren Wei wrote:
>> From: Longxuan Yu <ylong030@ucr.edu>
>>
>> vlan_dev_set_egress_priority() currently keeps cleared egress
>> priority mappings in the hash as tombstones. Repeated set/clear cycles
>> with distinct skb priorities therefore accumulate mapping nodes until
>> device teardown and leak memory.
>>
>> Delete mappings when vlan_prio is cleared instead of keeping tombstones.
>> Now that the egress mapping lists are RCU protected, the node can be
>> unlinked safely and freed after a grace period.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Cc: stable@kernel.org
>> Reported-by: Yifan Wu <yifanwucs@gmail.com>
>> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
>> Reported-by: Xin Liu <bird@lzu.edu.cn>
>> Co-developed-by: Yuan Tan <yuantan098@gmail.com>
>> Signed-off-by: Yuan Tan <yuantan098@gmail.com>
>> Signed-off-by: Longxuan Yu <ylong030@ucr.edu>
>> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
>> ---
>>  net/8021q/vlan_dev.c     | 20 ++++++++++++++------
>>  net/8021q/vlan_netlink.c |  4 ----
>>  2 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
>> index a5340932b657..7aa3af8b10ea 100644
>> --- a/net/8021q/vlan_dev.c
>> +++ b/net/8021q/vlan_dev.c
>> @@ -172,26 +172,34 @@ int vlan_dev_set_egress_priority(const struct net_device *dev,
>>  				 u32 skb_prio, u16 vlan_prio)
>>  {
>>  	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
>> +	struct vlan_priority_tci_mapping __rcu **mpp;
>>  	struct vlan_priority_tci_mapping *mp;
>>  	struct vlan_priority_tci_mapping *np;
>>  	u32 bucket = skb_prio & 0xF;
>>  	u32 vlan_qos = (vlan_prio << VLAN_PRIO_SHIFT) & VLAN_PRIO_MASK;
>>  
>>  	/* See if a priority mapping exists.. */
>> -	mp = rtnl_dereference(vlan->egress_priority_map[bucket]);
>> +	mpp = &vlan->egress_priority_map[bucket];
>> +	mp = rtnl_dereference(*mpp);
>>  	while (mp) {
>>  		if (mp->priority == skb_prio) {
>> -			if (mp->vlan_qos && !vlan_qos)
>> +			if (!vlan_qos) {
>> +				rcu_assign_pointer(*mpp, rtnl_dereference(mp->next));
>>  				vlan->nr_egress_mappings--;
>> -			else if (!mp->vlan_qos && vlan_qos)
>> -				vlan->nr_egress_mappings++;
>> -			WRITE_ONCE(mp->vlan_qos, vlan_qos);
>> +				kfree_rcu(mp, rcu);
>> +			} else {
>> +				WRITE_ONCE(mp->vlan_qos, vlan_qos);
>> +			}
>>  			return 0;
>>  		}
>> -		mp = rtnl_dereference(mp->next);
>> +		mpp = &mp->next;
>> +		mp = rtnl_dereference(*mpp);
>>  	}
> 
> Hi Ren,
> 
> Thanks for splitting up the patchset, it is very helpful to me.
> 
> It seems to me that the mpp/mp construct used is a bit complex and
> stems from the use of a hand-rolled list centred the next field of
> struct vlan_priority_tci_mapping.
> 
> I wonder if things can be simplified by moving to use a standardised
> list construct, such as an hlist. And the helpers available for using it.

I agree hlist_rcu usage would simplify the code above, but I fear it
will make this already relatively big change even bigger, as several
list traversing places will need to be updated accordingly.

I think conversion to hlist_rcu could/should be a net-next follow-up.

Thanks,

Paolo