netdev - Re: [PATCH net-next 2/2] net/sched: act_csum: don't use spinlock in the fast path

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: <20171213.162328.1374458006305010879.davem@davemloft.net>
Date:   Wed, 13 Dec 2017 16:23:28 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     dcaratti@...hat.com
Cc:     xiyou.wangcong@...il.com, jiri@...lanox.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] net/sched: act_csum: don't use spinlock
 in the fast path

From: Davide Caratti <dcaratti@...hat.com>
Date: Wed, 13 Dec 2017 10:48:38 +0100

> Then, in the data path, use READ_ONCE() to
> read those values, to avoid lock contention among multiple readers.
 ...
> @@ -544,14 +543,12 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
>  
>  	tcf_lastuse_update(&p->tcf_tm);
>  	bstats_cpu_update(this_cpu_ptr(p->common.cpu_bstats), skb);
> -	spin_lock(&p->tcf_lock);
> -	action = p->tcf_action;
> -	update_flags = p->update_flags;
> -	spin_unlock(&p->tcf_lock);
>  
> +	action = READ_ONCE(p->tcf_action);
>  	if (unlikely(action == TC_ACT_SHOT))
>  		goto drop;
>  
> +	update_flags = READ_ONCE(p->update_flags);
>  	switch (tc_skb_protocol(skb)) {
>  	case cpu_to_be16(ETH_P_IP):
>  		if (!tcf_csum_ipv4(skb, update_flags))

That's not why the lock is here.

We must read both action and flags atomically so that they are consistent
with eachother.

We must never use action from one configuration change and flags from
yet another.

Find a way to load both of these values with a single cpu load, then you
can legally remove the lock.