linux-kernel - Re: [PATCH RT 1/5] net: Properly annotate the try-lock for the seqlock

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]

Date:   Fri, 13 Nov 2020 11:06:53 -0600
From:   Tom Zanussi <zanussi@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>, linux-kernel@...r.kernel.org,
        linux-rt-users <linux-rt-users@...r.kernel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Carsten Emde <C.Emde@...dl.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        John Kacur <jkacur@...hat.com>, Daniel Wagner <wagi@...om.org>,
        "Srivatsa S. Bhat" <srivatsa@...il.mit.edu>,
        Mike Galbraith <efault@....de>, stable-rt@...r.kernel.org
Subject: Re: [PATCH RT 1/5] net: Properly annotate the try-lock for the
 seqlock

Hi Steve,

On Tue, 2020-11-10 at 10:38 -0500, Steven Rostedt wrote:
> 5.4.74-rt42-rc2 stable review patch.
> If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> 
> In patch
>    ("net/Qdisc: use a seqlock instead seqcount")
> 
> the seqcount has been replaced with a seqlock to allow to reader to
> boost the preempted writer.
> The try_write_seqlock() acquired the lock with a try-lock but the
> seqcount annotation was "lock".
> 
> Opencode write_seqcount_t_begin() and use the try-lock annotation for
> lockdep.
> 
> Reported-by: Mike Galbraith <efault@....de>
> Cc: stable-rt@...r.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> ---
>  include/linux/seqlock.h   |  9 ---------
>  include/net/sch_generic.h | 10 +++++++++-
>  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> index e5207897c33e..f390293974ea 100644
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -489,15 +489,6 @@ static inline void write_seqlock(seqlock_t *sl)
>  	__raw_write_seqcount_begin(&sl->seqcount);
>  }
>  
> -static inline int try_write_seqlock(seqlock_t *sl)
> -{
> -	if (spin_trylock(&sl->lock)) {
> -		__raw_write_seqcount_begin(&sl->seqcount);
> -		return 1;
> -	}
> -	return 0;
> -}
> -
>  static inline void write_sequnlock(seqlock_t *sl)
>  {
>  	__raw_write_seqcount_end(&sl->seqcount);
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index e6afb4b9cede..112d2dca8b08 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -168,8 +168,16 @@ static inline bool qdisc_run_begin(struct Qdisc
> *qdisc)
>  		return false;
>  	}
>  #ifdef CONFIG_PREEMPT_RT
> -	if (try_write_seqlock(&qdisc->running))
> +	if (spin_trylock(&qdisc->running.lock)) {
> +		seqcount_t *s = &qdisc->running.seqcount;
> +		/*
> +		 * Variant of write_seqcount_t_begin() telling lockdep
> that a
> +		 * trylock was attempted.
> +		 */
> +		__raw_write_seqcount_begin(s);
> +		seqcount_acquire(&s->dep_map, 0, 1, _RET_IP_);
>  		return true;
> +	}
>  	return false;
>  #else
>  	/* Variant of write_seqcount_begin() telling lockdep a trylock

I applied this to 4.19 and saw some splat with my 'debug-full'
configuration, so tried 5.4 and saw the same thing:

[   30.573698] BUG: workqueue leaked lock or atomic: kworker/1:4/0x00000000/143
                    last function: wireless_nlevent_process
[   30.573707] 1 lock held by kworker/1:4/143:
[   30.573708]  #0: ffffffff8e981d80 (noop_qdisc.running#2){+.+.}, at: __do_softirq+0xca/0x561
[   30.573715] CPU: 1 PID: 143 Comm: kworker/1:4 Not tainted 5.4.74-rt42-rt-test-full-debug #1
[   30.573716] Hardware name: LENOVO 4236L51/4236L51, BIOS 83ET59WW (1.29 ) 06/01/2011
[   30.573720] Workqueue: events wireless_nlevent_process
[   30.573721] Call Trace:
[   30.573724]  dump_stack+0x71/0x9b
[   30.573728]  process_one_work+0x533/0x760
[   30.573731]  worker_thread+0x39/0x3f0
[   30.573733]  ? process_one_work+0x760/0x760
[   30.573734]  kthread+0x165/0x180
[   30.573736]  ? __kthread_create_on_node+0x180/0x180
[   30.573737]  ret_from_fork+0x3a/0x50
[   30.629329] wlp3s0: authenticate with 84:1b:5e:41:5e:4d
[   30.634864] wlp3s0: send auth to 84:1b:5e:41:5e:4d (try 1/3)
[   30.638433] wlp3s0: authenticated
[   30.642250] wlp3s0: associate with 84:1b:5e:41:5e:4d (try 1/3)
[   30.645704] wlp3s0: RX AssocResp from 84:1b:5e:41:5e:4d (capab=0x411 status=0 aid=6)
[   30.650803] wlp3s0: associated

[   30.656764] ================================================
[   30.656765] WARNING: lock held when returning to user space!
[   30.656766] 5.4.74-rt42-rt-test-full-debug #1 Not tainted
[   30.656767] ------------------------------------------------
[   30.656768] wpa_supplicant/836 is leaving the kernel with locks still held!
[   30.656769] 1 lock held by wpa_supplicant/836:
[   30.656770]  #0: ffff98f1c9622280 (&dev->qdisc_running_key){+.+.}, at: packet_sendmsg+0xec1/0x1ad0

Let me know if you want me to send you my .config or the full output (a
bunch more of the above).

Thanks,

Tom