Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220706234003.66760-1-kuniyu@amazon.com> <20220706234003.66760-11-kuniyu@amazon.com>
In-Reply-To: <20220706234003.66760-11-kuniyu@amazon.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Thu, 7 Jul 2022 15:15:52 -0400
Message-ID: <CAHC9VhQMigGi65-j0c9WBN+dWLjjaYqTti-eP99c1RRrQzWj5g@mail.gmail.com>
Subject: Re: [PATCH v2 net 10/12] cipso: Fix data-races around sysctl.
To:     Kuniyuki Iwashima <kuniyu@amazon.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Iurii Zaikin <yzaikin@google.com>,
        Kuniyuki Iwashima <kuni1840@gmail.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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