| Message ID | 20220706234003.66760-11-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dd44f04b9214adb68ef5684ae87a81ba03632250 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sysctl: Fix data-races around ipv4_table. | expand |
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
From: Paul Moore <paul@paul-moore.com> Date: Thu, 7 Jul 2022 15:15:52 -0400 > 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> I was wondering if both CC and Acked-by should stay in each commit, but will do so in the next time. Thank you for taking a look!
On Thu, 7 Jul 2022 15:15:37 -0700 Kuniyuki Iwashima wrote: > I was wondering if both CC and Acked-by should stay in each commit, but > will do so in the next time. For Paul only, don't take that as general advice.
From: Jakub Kicinski <kuba@kernel.org> Date: Thu, 7 Jul 2022 17:24:24 -0700 > On Thu, 7 Jul 2022 15:15:37 -0700 Kuniyuki Iwashima wrote: > > I was wondering if both CC and Acked-by should stay in each commit, but > > will do so in the next time. > > For Paul only, don't take that as general advice. ...got it, thanks :)
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;
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> --- Documentation/networking/ip-sysctl.rst | 2 +- net/ipv4/cipso_ipv4.c | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-)