Messages in this thread | | | From | Paul Moore <> | Date | Thu, 7 Jul 2022 15:15:52 -0400 | Subject | Re: [PATCH v2 net 10/12] cipso: Fix data-races around sysctl. |
| |
On Wed, Jul 6, 2022 at 7:43 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > While reading cipso sysctl variables, they can be changed concurrently. > So, we need to add READ_ONCE() to avoid data-races. > > Fixes: 446fda4f2682 ("[NetLabel]: CIPSOv4 engine") > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- > CC: Paul Moore <paul@paul-moore.com>
Thanks for the patch, this looks good to me. However, in the future you should probably drop the extra "---" separator (just leave the one before the diffstat below) and move my "Cc:" up above "Fixes:".
Acked-by: Paul Moore <paul@paul-moore.com>
> --- > Documentation/networking/ip-sysctl.rst | 2 +- > net/ipv4/cipso_ipv4.c | 12 +++++++----- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst > index 9f41961d11d5..0e58001f8580 100644 > --- a/Documentation/networking/ip-sysctl.rst > +++ b/Documentation/networking/ip-sysctl.rst > @@ -1085,7 +1085,7 @@ cipso_cache_enable - BOOLEAN > cipso_cache_bucket_size - INTEGER > The CIPSO label cache consists of a fixed size hash table with each > hash bucket containing a number of cache entries. This variable limits > - the number of entries in each hash bucket; the larger the value the > + the number of entries in each hash bucket; the larger the value is, the > more CIPSO label mappings that can be cached. When the number of > entries in a given hash bucket reaches this limit adding new entries > causes the oldest entry in the bucket to be removed to make room. > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > index 62d5f99760aa..6cd3b6c559f0 100644 > --- a/net/ipv4/cipso_ipv4.c > +++ b/net/ipv4/cipso_ipv4.c > @@ -239,7 +239,7 @@ static int cipso_v4_cache_check(const unsigned char *key, > struct cipso_v4_map_cache_entry *prev_entry = NULL; > u32 hash; > > - if (!cipso_v4_cache_enabled) > + if (!READ_ONCE(cipso_v4_cache_enabled)) > return -ENOENT; > > hash = cipso_v4_map_cache_hash(key, key_len); > @@ -296,13 +296,14 @@ static int cipso_v4_cache_check(const unsigned char *key, > int cipso_v4_cache_add(const unsigned char *cipso_ptr, > const struct netlbl_lsm_secattr *secattr) > { > + int bkt_size = READ_ONCE(cipso_v4_cache_bucketsize); > int ret_val = -EPERM; > u32 bkt; > struct cipso_v4_map_cache_entry *entry = NULL; > struct cipso_v4_map_cache_entry *old_entry = NULL; > u32 cipso_ptr_len; > > - if (!cipso_v4_cache_enabled || cipso_v4_cache_bucketsize <= 0) > + if (!READ_ONCE(cipso_v4_cache_enabled) || bkt_size <= 0) > return 0; > > cipso_ptr_len = cipso_ptr[1]; > @@ -322,7 +323,7 @@ int cipso_v4_cache_add(const unsigned char *cipso_ptr, > > bkt = entry->hash & (CIPSO_V4_CACHE_BUCKETS - 1); > spin_lock_bh(&cipso_v4_cache[bkt].lock); > - if (cipso_v4_cache[bkt].size < cipso_v4_cache_bucketsize) { > + if (cipso_v4_cache[bkt].size < bkt_size) { > list_add(&entry->list, &cipso_v4_cache[bkt].list); > cipso_v4_cache[bkt].size += 1; > } else { > @@ -1199,7 +1200,8 @@ static int cipso_v4_gentag_rbm(const struct cipso_v4_doi *doi_def, > /* This will send packets using the "optimized" format when > * possible as specified in section 3.4.2.6 of the > * CIPSO draft. */ > - if (cipso_v4_rbm_optfmt && ret_val > 0 && ret_val <= 10) > + if (READ_ONCE(cipso_v4_rbm_optfmt) && ret_val > 0 && > + ret_val <= 10) > tag_len = 14; > else > tag_len = 4 + ret_val; > @@ -1603,7 +1605,7 @@ int cipso_v4_validate(const struct sk_buff *skb, unsigned char **option) > * all the CIPSO validations here but it doesn't > * really specify _exactly_ what we need to validate > * ... so, just make it a sysctl tunable. */ > - if (cipso_v4_rbm_strictvalid) { > + if (READ_ONCE(cipso_v4_rbm_strictvalid)) { > if (cipso_v4_map_lvl_valid(doi_def, > tag[3]) < 0) { > err_offset = opt_iter + 3; > -- > 2.30.2
-- paul-moore.com
| |