Re: [PATCH net v7 1/2] ipv6: flowlabel: take ip6_fl_lock across mem_check and fl_intern

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Maoyi Xie wrote:
> mem_check() in net/ipv6/ip6_flowlabel.c reads fl_size without
> holding ip6_fl_lock. fl_intern() takes the lock immediately
> afterwards. The two checks therefore race against concurrent
> fl_intern, ip6_fl_gc and ip6_fl_purge writers, which makes the
> mem_check budget check approximate.
> 
> Move spin_lock_bh(&ip6_fl_lock) and the matching unlock from
> fl_intern() into its only caller ipv6_flowlabel_get(). The
> mem_check() call now runs under the same critical section as the
> fl_intern() insert, so the budget check is exact.
> 
> With all writers and the read of fl_size under ip6_fl_lock,
> convert fl_size from atomic_t to plain int. The four sites that
> update or read fl_size are fl_intern (insert path), ip6_fl_gc
> (garbage collector, the !sched check and the per-entry decrement),
> ip6_fl_purge (per-netns purge), and mem_check (budget check), and
> all four now run under ip6_fl_lock.
> 
> This is a prerequisite for adding a per-netns budget alongside
> fl_size. The follow-up patch adds netns_ipv6::flowlabel_count and
> folds it into mem_check().
> 

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

> Suggested-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Maoyi Xie <maoyi.xie@ntu.edu.sg>

Please update either your git config name or Signed-off-by to make
sure that the two are the same. Not sure whether that requires a
respin.

With those asides

Reviewed-by: Willem de Bruijn <willemb@google.com>

Thanks for fixing this along with your main fix.

> ---
>  net/ipv6/ip6_flowlabel.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
> index c92f98c6f..43b5e9ce9 100644
> --- a/net/ipv6/ip6_flowlabel.c
> +++ b/net/ipv6/ip6_flowlabel.c
> @@ -40,7 +40,7 @@
>  #define FL_HASH_MASK	255
>  #define FL_HASH(l)	(ntohl(l)&FL_HASH_MASK)
>  
> -static atomic_t fl_size = ATOMIC_INIT(0);
> +static int fl_size;
>  static struct ip6_flowlabel __rcu *fl_ht[FL_HASH_MASK+1];
>  
>  static void ip6_fl_gc(struct timer_list *unused);
> @@ -163,7 +163,7 @@ static void ip6_fl_gc(struct timer_list *unused)
>  				if (time_after_eq(now, ttd)) {
>  					*flp = fl->next;
>  					fl_free(fl);
> -					atomic_dec(&fl_size);
> +					fl_size--;
>  					continue;
>  				}
>  				if (!sched || time_before(ttd, sched))
> @@ -172,7 +172,7 @@ static void ip6_fl_gc(struct timer_list *unused)
>  			flp = &fl->next;
>  		}
>  	}
> -	if (!sched && atomic_read(&fl_size))
> +	if (!sched && fl_size)
>  		sched = now + FL_MAX_LINGER;
>  	if (sched) {
>  		mod_timer(&ip6_fl_gc_timer, sched);
> @@ -196,7 +196,7 @@ static void __net_exit ip6_fl_purge(struct net *net)
>  			    atomic_read(&fl->users) == 0) {
>  				*flp = fl->next;
>  				fl_free(fl);
> -				atomic_dec(&fl_size);
> +				fl_size--;
>  				continue;
>  			}
>  			flp = &fl->next;
> @@ -205,6 +205,7 @@ static void __net_exit ip6_fl_purge(struct net *net)
>  	spin_unlock_bh(&ip6_fl_lock);
>  }
>  
> +/* Caller must hold ip6_fl_lock. */

nit: lockdep_assert_held as used below is preferable over comments


> @@ -464,10 +459,14 @@ fl_create(struct net *net, struct sock *sk, struct in6_flowlabel_req *freq,
>  
>  static int mem_check(struct sock *sk)
>  {
> -	int room = FL_MAX_SIZE - atomic_read(&fl_size);
> +	int room;
>  	struct ipv6_fl_socklist *sfl;
>  	int count = 0;
>  
> +	lockdep_assert_held(&ip6_fl_lock);
> +
> +	room = FL_MAX_SIZE - fl_size;
> +
>  	if (room > FL_MAX_SIZE - FL_MAX_PER_SOCK)
>  		return 0;
>