netdev - Re: [PATCH net-next 02/14] net_sched: cake: implement lockless cake

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240417083549.GA3846178@kernel.org>
Date: Wed, 17 Apr 2024 09:35:49 +0100
From: Simon Horman <horms@...nel.org>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	netdev@...r.kernel.org, eric.dumazet@...il.com,
	Toke Høiland-Jørgensen <toke@...e.dk>,
	cake@...ts.bufferbloat.net
Subject: Re: [PATCH net-next 02/14] net_sched: cake: implement lockless
 cake_dump()

+ Toke Høiland-Jørgensen <toke@...e.dk>
  cake@...ts.bufferbloat.net

On Mon, Apr 15, 2024 at 01:20:42PM +0000, Eric Dumazet wrote:
> Instead of relying on RTNL, cake_dump() can use READ_ONCE()
> annotations, paired with WRITE_ONCE() ones in cake_change().
> 
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>

...

> @@ -2774,68 +2783,71 @@ static int cake_dump(struct Qdisc *sch, struct sk_buff *skb)
>  {
>  	struct cake_sched_data *q = qdisc_priv(sch);
>  	struct nlattr *opts;
> +	u16 rate_flags;
>  
>  	opts = nla_nest_start_noflag(skb, TCA_OPTIONS);
>  	if (!opts)
>  		goto nla_put_failure;
>  
> -	if (nla_put_u64_64bit(skb, TCA_CAKE_BASE_RATE64, q->rate_bps,
> -			      TCA_CAKE_PAD))
> +	if (nla_put_u64_64bit(skb, TCA_CAKE_BASE_RATE64,
> +			      READ_ONCE(q->rate_bps), TCA_CAKE_PAD))
>  		goto nla_put_failure;
>  
>  	if (nla_put_u32(skb, TCA_CAKE_FLOW_MODE,
> -			q->flow_mode & CAKE_FLOW_MASK))
> +			READ_ONCE(q->flow_mode) & CAKE_FLOW_MASK))
>  		goto nla_put_failure;

Hi Eric,

q->flow_mode is read twice in this function. Once here...

>  
> -	if (nla_put_u32(skb, TCA_CAKE_RTT, q->interval))
> +	if (nla_put_u32(skb, TCA_CAKE_RTT, READ_ONCE(q->interval)))
>  		goto nla_put_failure;
>  
> -	if (nla_put_u32(skb, TCA_CAKE_TARGET, q->target))
> +	if (nla_put_u32(skb, TCA_CAKE_TARGET, READ_ONCE(q->target)))
>  		goto nla_put_failure;
>  
> -	if (nla_put_u32(skb, TCA_CAKE_MEMORY, q->buffer_config_limit))
> +	if (nla_put_u32(skb, TCA_CAKE_MEMORY,
> +			READ_ONCE(q->buffer_config_limit)))
>  		goto nla_put_failure;
>  
> +	rate_flags = READ_ONCE(q->rate_flags);
>  	if (nla_put_u32(skb, TCA_CAKE_AUTORATE,
> -			!!(q->rate_flags & CAKE_FLAG_AUTORATE_INGRESS)))
> +			!!(rate_flags & CAKE_FLAG_AUTORATE_INGRESS)))
>  		goto nla_put_failure;
>  
>  	if (nla_put_u32(skb, TCA_CAKE_INGRESS,
> -			!!(q->rate_flags & CAKE_FLAG_INGRESS)))
> +			!!(rate_flags & CAKE_FLAG_INGRESS)))
>  		goto nla_put_failure;
>  
> -	if (nla_put_u32(skb, TCA_CAKE_ACK_FILTER, q->ack_filter))
> +	if (nla_put_u32(skb, TCA_CAKE_ACK_FILTER, READ_ONCE(q->ack_filter)))
>  		goto nla_put_failure;
>  
>  	if (nla_put_u32(skb, TCA_CAKE_NAT,
> -			!!(q->flow_mode & CAKE_FLOW_NAT_FLAG)))
> +			!!(READ_ONCE(q->flow_mode) & CAKE_FLOW_NAT_FLAG)))
>  		goto nla_put_failure;

... and once here.

I am assuming that it isn't a big deal, but perhaps it is better to save
q->flow_mode into a local variable.

Also, more importantly, q->flow_mode does not seem to be handled
using WRITE_ONCE() in cake_change(). It's a non-trivial case,
which I guess is well served by a mechanism built around a local variable.

...