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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] calipso: double READ_ONCE() of bucketsize in
  calipso_cache_add() causes TOCTOU
- [Low] calipso: missing Fixes trailer for commit 4fee5242bf41 in commit
  message
--

commit 430c1b1d214e9e8a15979062b88b1428916c12ef
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

> calipso: annotate data-races around cache sysctls
> 
> calipso_cache_enabled and calipso_cache_bucketsize are read without any
> lock while their sysctl handlers update them with WRITE_ONCE(). Add
> READ_ONCE() to the readers.
> 
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>

[Severity: Low]
Should this include a Fixes tag for the commit that introduced the
unannotated sysctl reads?

  Fixes: 4fee5242bf41 ("calipso: Add a label cache.")

> 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: Medium]
Can reloading calipso_cache_bucketsize with a second READ_ONCE() corrupt
calipso_cache[bkt].list when the sysctl changes concurrently?

net/ipv6/calipso.c:calipso_cache_add() {
	...
	} else {
		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);
	}
	...
}

The else branch assumes the bucket list is non-empty because
calipso_cache_bucketsize was checked to be positive at function entry.
With two separate READ_ONCE() calls, a concurrent sysctl write can change
calipso_cache_bucketsize between them while calipso_cache[bkt].size is 0:

CPU0                                    CPU1
calipso_cache_add()
  READ_ONCE(calipso_cache_bucketsize)
    -> > 0
                                        proc_dointvec()
                                          WRITE_ONCE(calipso_cache_bucketsize, 0)
  spin_lock_bh(&calipso_cache[bkt].lock)
  calipso_cache[bkt].size <
    READ_ONCE(calipso_cache_bucketsize)
    -> 0 < 0 (false)
  old_entry = list_entry(...)
  list_del(&old_entry->list)
  calipso_cache_entry_free(old_entry)   <-- frees list head as entry

When calipso_cache[bkt].size is 0 and the second READ_ONCE() observes 0,
the else branch unlinks the empty list head and passes an interior pointer
of calipso_cache to calipso_cache_entry_free().  If the second READ_ONCE()
observes a negative value, comparing u32 calipso_cache[bkt].size against a
negative int promotes it to a large unsigned value and bypasses the bucket
size limit.

Snapshotting READ_ONCE(calipso_cache_bucketsize) into a local int variable
once and reusing it in both checks, as cipso_v4_cache_add() does in
dd44f04b9214 ("cipso: Fix data-races around sysctl."), avoids the second
load.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109025425.328445-1-jiayuan.chen@linux.dev?part=5