Re: [PATCH net-next 05/14] calipso: annotate data-races around cache sysctls

[Simon Horman <horms@kernel.org>]

On Tue, Nov 10, 2026 at 05:53:19AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] calipso_cache_add() in net/ipv6/calipso.c still reads
>   calipso_cache_bucketsize twice, and the two reads can disagree.
> 
> --
> 
> calipso: annotate data-races around cache sysctls
> 
> This commit adds READ_ONCE() to the lockless readers of
> calipso_cache_enabled and calipso_cache_bucketsize in net/ipv6/calipso.c.
> The sysctl handlers update these variables with WRITE_ONCE().
> 
> > diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
> > index e399a42e30839..5e63212b60163 100644
> > --- a/net/ipv6/calipso.c
> > +++ b/net/ipv6/calipso.c
> 
> [ ... ]
> 
> > @@ -269,7 +269,7 @@ static int calipso_cache_add(const unsigned char *calipso_ptr,
> >  	struct calipso_map_cache_entry *old_entry = NULL;
> >  	u32 calipso_ptr_len;
> >  
> > -	if (!calipso_cache_enabled || calipso_cache_bucketsize <= 0)
> > +	if (!READ_ONCE(calipso_cache_enabled) || READ_ONCE(calipso_cache_bucketsize) <= 0)
> >  		return 0;
> >  
> >  	calipso_ptr_len = calipso_ptr[1];
> > @@ -289,7 +289,7 @@ static int calipso_cache_add(const unsigned char *calipso_ptr,
> >  
> >  	bkt = entry->hash & (CALIPSO_CACHE_BUCKETS - 1);
> >  	spin_lock_bh(&calipso_cache[bkt].lock);
> > -	if (calipso_cache[bkt].size < calipso_cache_bucketsize) {
> > +	if (calipso_cache[bkt].size < READ_ONCE(calipso_cache_bucketsize)) {
> >  		list_add(&entry->list, &calipso_cache[bkt].list);
> >  		calipso_cache[bkt].size += 1;
> >  	} else {
> 
> [Severity: High]
> Can these two separate READ_ONCE() loads of calipso_cache_bucketsize in
> calipso_cache_add() return different values?
> 
> The first load is unlocked and only guards the early return. The second
> load comes later, under the bucket spinlock, after the GFP_ATOMIC
> allocations, jhash() and refcount_inc().
> 
> The bucket lock does not cover the sysctl. Also, calipso_cache_bucket_size
> in net/ipv6/sysctl_net_ipv6.c uses plain proc_dointvec with no
> extra1/extra2 bounds, so 0 or a negative value can be written during that
> window.
> 
> Suppose three things happen together:
> 
>   - the first load sees a positive value
>   - a concurrent write of 0 lands before the second load
>   - the hashed bucket is empty (size == 0)
> 
> Then the size check is false, and the eviction branch runs on an empty
> list:
> 
>     old_entry = list_entry(calipso_cache[bkt].list.prev,
>                            struct calipso_map_cache_entry, list);
>     list_del(&old_entry->list);
>     list_add(&entry->list, &calipso_cache[bkt].list);
>     calipso_cache_entry_free(old_entry);
> 
> Here old_entry is container_of() applied to the bucket's own list head.
> list_del() poisons the head, and list_add() then dereferences
> LIST_POISON1.
> 
> Wouldn't that oops in softirq context with the bucket lock held?
> 
> If execution got past that point, calipso_cache_entry_free() would free
> whatever sits at the key and lsm_data offsets of the fake entry. Those
> offsets overlap neighbouring buckets. It would then kfree() a pointer into
> the middle of the calipso_cache array.
> 
> Separately, if only the second load sees a negative value, it is
> converted to a huge u32 limit and the bucket size cap is skipped.
> 
> This window existed before this patch, since spin_lock_bh() already forced
> a reload. However, this commit is meant to fix the racy reads of this
> exact variable. Could it take a single snapshot instead? That is what
> cipso_v4_cache_add() has done since commit dd44f04b9214ad ("cipso: Fix
> data-races around sysctl."):
> 
> net/ipv4/cipso_ipv4.c:cipso_v4_cache_add() {
> 	int bkt_size = READ_ONCE(cipso_v4_cache_bucketsize);
> 	...
> 	if (!READ_ONCE(cipso_v4_cache_enabled) || bkt_size <= 0)
> 		return 0;
> 	...
> 	if (cipso_v4_cache[bkt].size < bkt_size) {
> 	...
> }
> 
> No later patch in the series changes net/ipv6/calipso.c.

This problem looks legit to me.

-- 
pw-bot: changes-requested