public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming
@ 2024-07-10  9:31 Daniel Vetter
  2024-07-10  9:31 ` [PATCH 2/2] bcachefs: only console_trylock in bch2_print_string_as_lines Daniel Vetter
  2024-07-10 11:38 ` [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming Christian König
  0 siblings, 2 replies; 11+ messages in thread
From: Daniel Vetter @ 2024-07-10  9:31 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: Intel Graphics Development, Daniel Vetter, Daniel Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Sumit Semwal,
	Christian König, linux-media, linaro-mm-sig

We already teach lockdep that dma_resv nests within drm_modeset_lock,
but there's a lot more: All drm kms ioctl rely on being able to
put/get_user while holding modeset locks, so we really need a
might_fault in there too to complete the picture. Add it.

Motivated by a syzbot report that blew up on bcachefs doing an
unconditional console_lock way deep in the locking hierarchy, and
lockdep only noticing the depency loop in a drm ioctl instead of much
earlier. This annotation will make sure such issues have a much harder
time escaping.

References: https://lore.kernel.org/dri-devel/00000000000073db8b061cd43496@google.com/
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
---
 drivers/gpu/drm/drm_mode_config.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 568972258222..37d2e0a4ef4b 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev)
 		if (ret == -EDEADLK)
 			ret = drm_modeset_backoff(&modeset_ctx);
 
+		might_fault();
+
 		ww_acquire_init(&resv_ctx, &reservation_ww_class);
 		ret = dma_resv_lock(&resv, &resv_ctx);
 		if (ret == -EDEADLK)
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] bcachefs: only console_trylock in bch2_print_string_as_lines
  2024-07-10  9:31 [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming Daniel Vetter
@ 2024-07-10  9:31 ` Daniel Vetter
  2024-07-10 11:02   ` John Ogness
  2024-07-10 13:03   ` [PATCH] bcachefs: no console_lock " Daniel Vetter
  2024-07-10 11:38 ` [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming Christian König
  1 sibling, 2 replies; 11+ messages in thread
From: Daniel Vetter @ 2024-07-10  9:31 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: Intel Graphics Development, Daniel Vetter,
	syzbot+6cebc1af246fe020a2f0, Daniel Vetter, stable,
	Kent Overstreet, Brian Foster, linux-bcachefs, Petr Mladek,
	Steven Rostedt, John Ogness, Sergey Senozhatsky

console_lock is the outermost subsystem lock for a lot of subsystems,
which means get/put_user must nest within. Which means it cannot be
acquired somewhere deeply nested in other locks, and most definitely
not while holding fs locks potentially needed to resolve faults.

console_trylock is the best we can do here.

Including printk folks since even trylock feels realyl iffy here to
me.

Reported-by: syzbot+6cebc1af246fe020a2f0@syzkaller.appspotmail.com
References: https://lore.kernel.org/dri-devel/00000000000026c1ff061cd0de12@google.com/
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Fixes: a8f354284304 ("bcachefs: bch2_print_string_as_lines()")
Cc: <stable@vger.kernel.org> # v6.7+
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Brian Foster <bfoster@redhat.com>
Cc: linux-bcachefs@vger.kernel.org
Cc: Petr Mladek <pmladek@suse.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 fs/bcachefs/util.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/bcachefs/util.c b/fs/bcachefs/util.c
index de331dec2a99..02381c653603 100644
--- a/fs/bcachefs/util.c
+++ b/fs/bcachefs/util.c
@@ -255,13 +255,14 @@ void bch2_prt_u64_base2(struct printbuf *out, u64 v)
 void bch2_print_string_as_lines(const char *prefix, const char *lines)
 {
 	const char *p;
+	int locked;
 
 	if (!lines) {
 		printk("%s (null)\n", prefix);
 		return;
 	}
 
-	console_lock();
+	locked = console_trylock();
 	while (1) {
 		p = strchrnul(lines, '\n');
 		printk("%s%.*s\n", prefix, (int) (p - lines), lines);
@@ -269,7 +270,8 @@ void bch2_print_string_as_lines(const char *prefix, const char *lines)
 			break;
 		lines = p + 1;
 	}
-	console_unlock();
+	if (locked)
+		console_unlock();
 }
 
 int bch2_save_backtrace(bch_stacktrace *stack, struct task_struct *task, unsigned skipnr,
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] bcachefs: only console_trylock in bch2_print_string_as_lines
  2024-07-10  9:31 ` [PATCH 2/2] bcachefs: only console_trylock in bch2_print_string_as_lines Daniel Vetter
@ 2024-07-10 11:02   ` John Ogness
  2024-07-10 13:03   ` [PATCH] bcachefs: no console_lock " Daniel Vetter
  1 sibling, 0 replies; 11+ messages in thread
From: John Ogness @ 2024-07-10 11:02 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: Intel Graphics Development, Daniel Vetter,
	syzbot+6cebc1af246fe020a2f0, Daniel Vetter, stable,
	Kent Overstreet, Brian Foster, linux-bcachefs, Petr Mladek,
	Steven Rostedt, Sergey Senozhatsky

On 2024-07-10, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> console_lock is the outermost subsystem lock for a lot of subsystems,
> which means get/put_user must nest within. Which means it cannot be
> acquired somewhere deeply nested in other locks, and most definitely
> not while holding fs locks potentially needed to resolve faults.
>
> console_trylock is the best we can do here.
>
> Including printk folks since even trylock feels realyl iffy here to
> me.

Using the console lock here at all is wrong. The console lock does not
prevent other CPUs from calling printk() and inserting lines in between.

There is no way to guarantee a contiguous ringbuffer block using
multiple printk() calls.

The console_lock usage should be removed.

John Ogness

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming
  2024-07-10  9:31 [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming Daniel Vetter
  2024-07-10  9:31 ` [PATCH 2/2] bcachefs: only console_trylock in bch2_print_string_as_lines Daniel Vetter
@ 2024-07-10 11:38 ` Christian König
  2024-07-10 11:58   ` Daniel Vetter
  1 sibling, 1 reply; 11+ messages in thread
From: Christian König @ 2024-07-10 11:38 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: Intel Graphics Development, Daniel Vetter, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, Sumit Semwal, linux-media,
	linaro-mm-sig

Am 10.07.24 um 11:31 schrieb Daniel Vetter:
> We already teach lockdep that dma_resv nests within drm_modeset_lock,
> but there's a lot more: All drm kms ioctl rely on being able to
> put/get_user while holding modeset locks, so we really need a
> might_fault in there too to complete the picture. Add it.

Mhm, lockdep should be able to deduce that when there might be faults 
under the dma_resv lock there might also be faults under the 
drm_modeset_lock.

>
> Motivated by a syzbot report that blew up on bcachefs doing an
> unconditional console_lock way deep in the locking hierarchy, and
> lockdep only noticing the depency loop in a drm ioctl instead of much
> earlier. This annotation will make sure such issues have a much harder
> time escaping.
>
> References: https://lore.kernel.org/dri-devel/00000000000073db8b061cd43496@google.com/
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org

On the other hand pointing it out explicitly doesn't hurts us at all, so 
Reviewed-by: Christian König <christian.koenig@amd.com>.

Regards,
Christian.

> ---
>   drivers/gpu/drm/drm_mode_config.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 568972258222..37d2e0a4ef4b 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev)
>   		if (ret == -EDEADLK)
>   			ret = drm_modeset_backoff(&modeset_ctx);
>   
> +		might_fault();
> +
>   		ww_acquire_init(&resv_ctx, &reservation_ww_class);
>   		ret = dma_resv_lock(&resv, &resv_ctx);
>   		if (ret == -EDEADLK)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming
  2024-07-10 11:38 ` [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming Christian König
@ 2024-07-10 11:58   ` Daniel Vetter
  2024-07-10 12:40     ` Christian König
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2024-07-10 11:58 UTC (permalink / raw)
  To: Christian König
  Cc: DRI Development, LKML, Intel Graphics Development, Daniel Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Sumit Semwal,
	linux-media, linaro-mm-sig

On Wed, 10 Jul 2024 at 13:39, Christian König <christian.koenig@amd.com> wrote:
>
> Am 10.07.24 um 11:31 schrieb Daniel Vetter:
> > We already teach lockdep that dma_resv nests within drm_modeset_lock,
> > but there's a lot more: All drm kms ioctl rely on being able to
> > put/get_user while holding modeset locks, so we really need a
> > might_fault in there too to complete the picture. Add it.
>
> Mhm, lockdep should be able to deduce that when there might be faults
> under the dma_resv lock there might also be faults under the
> drm_modeset_lock.

You're not allowed to take a fault under dma_resv, because drivers
might need to take that lock to handle faults. So unfortunately in our
combined lockdep priming, there really seems to be no chain yet that
teaches about faults possibly happening while holding
drm_modeset_lock.
-Sima

>
> >
> > Motivated by a syzbot report that blew up on bcachefs doing an
> > unconditional console_lock way deep in the locking hierarchy, and
> > lockdep only noticing the depency loop in a drm ioctl instead of much
> > earlier. This annotation will make sure such issues have a much harder
> > time escaping.
> >
> > References: https://lore.kernel.org/dri-devel/00000000000073db8b061cd43496@google.com/
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: linux-media@vger.kernel.org
> > Cc: linaro-mm-sig@lists.linaro.org
>
> On the other hand pointing it out explicitly doesn't hurts us at all, so
> Reviewed-by: Christian König <christian.koenig@amd.com>.
>
> Regards,
> Christian.
>
> > ---
> >   drivers/gpu/drm/drm_mode_config.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 568972258222..37d2e0a4ef4b 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev)
> >               if (ret == -EDEADLK)
> >                       ret = drm_modeset_backoff(&modeset_ctx);
> >
> > +             might_fault();
> > +
> >               ww_acquire_init(&resv_ctx, &reservation_ww_class);
> >               ret = dma_resv_lock(&resv, &resv_ctx);
> >               if (ret == -EDEADLK)
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming
  2024-07-10 11:58   ` Daniel Vetter
@ 2024-07-10 12:40     ` Christian König
  2024-07-15 10:08       ` Daniel Vetter
  0 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2024-07-10 12:40 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: DRI Development, LKML, Intel Graphics Development, Daniel Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Sumit Semwal,
	linux-media, linaro-mm-sig

Am 10.07.24 um 13:58 schrieb Daniel Vetter:
> On Wed, 10 Jul 2024 at 13:39, Christian König <christian.koenig@amd.com> wrote:
>> Am 10.07.24 um 11:31 schrieb Daniel Vetter:
>>> We already teach lockdep that dma_resv nests within drm_modeset_lock,
>>> but there's a lot more: All drm kms ioctl rely on being able to
>>> put/get_user while holding modeset locks, so we really need a
>>> might_fault in there too to complete the picture. Add it.
>> Mhm, lockdep should be able to deduce that when there might be faults
>> under the dma_resv lock there might also be faults under the
>> drm_modeset_lock.
> You're not allowed to take a fault under dma_resv, because drivers
> might need to take that lock to handle faults. So unfortunately in our
> combined lockdep priming, there really seems to be no chain yet that
> teaches about faults possibly happening while holding
> drm_modeset_lock.

Ah, of course! You are right, it was just the other way around.

Thanks,
Christian.

> -Sima
>
>>> Motivated by a syzbot report that blew up on bcachefs doing an
>>> unconditional console_lock way deep in the locking hierarchy, and
>>> lockdep only noticing the depency loop in a drm ioctl instead of much
>>> earlier. This annotation will make sure such issues have a much harder
>>> time escaping.
>>>
>>> References: https://lore.kernel.org/dri-devel/00000000000073db8b061cd43496@google.com/
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Maxime Ripard <mripard@kernel.org>
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>>> Cc: "Christian König" <christian.koenig@amd.com>
>>> Cc: linux-media@vger.kernel.org
>>> Cc: linaro-mm-sig@lists.linaro.org
>> On the other hand pointing it out explicitly doesn't hurts us at all, so
>> Reviewed-by: Christian König <christian.koenig@amd.com>.
>>
>> Regards,
>> Christian.
>>
>>> ---
>>>    drivers/gpu/drm/drm_mode_config.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>>> index 568972258222..37d2e0a4ef4b 100644
>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>> @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev)
>>>                if (ret == -EDEADLK)
>>>                        ret = drm_modeset_backoff(&modeset_ctx);
>>>
>>> +             might_fault();
>>> +
>>>                ww_acquire_init(&resv_ctx, &reservation_ww_class);
>>>                ret = dma_resv_lock(&resv, &resv_ctx);
>>>                if (ret == -EDEADLK)
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] bcachefs: no console_lock in bch2_print_string_as_lines
  2024-07-10  9:31 ` [PATCH 2/2] bcachefs: only console_trylock in bch2_print_string_as_lines Daniel Vetter
  2024-07-10 11:02   ` John Ogness
@ 2024-07-10 13:03   ` Daniel Vetter
  2024-07-10 14:01     ` John Ogness
  2024-07-10 14:13     ` John Ogness
  1 sibling, 2 replies; 11+ messages in thread
From: Daniel Vetter @ 2024-07-10 13:03 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: Intel Graphics Development, Daniel Vetter,
	syzbot+6cebc1af246fe020a2f0, Daniel Vetter, stable,
	Kent Overstreet, Brian Foster, linux-bcachefs, Petr Mladek,
	Steven Rostedt, John Ogness, Sergey Senozhatsky

console_lock is the outermost subsystem lock for a lot of subsystems,
which means get/put_user must nest within. Which means it cannot be
acquired somewhere deeply nested in other locks, and most definitely
not while holding fs locks potentially needed to resolve faults.

console_trylock is the best we can do here. But John pointed out on a
previous version that this is futile:

"Using the console lock here at all is wrong. The console lock does not
prevent other CPUs from calling printk() and inserting lines in between.

"There is no way to guarantee a contiguous ringbuffer block using
multiple printk() calls.

"The console_lock usage should be removed."

https://lore.kernel.org/lkml/87frsh33xp.fsf@jogness.linutronix.de/

Do that.

Reported-by: syzbot+6cebc1af246fe020a2f0@syzkaller.appspotmail.com
References: https://lore.kernel.org/dri-devel/00000000000026c1ff061cd0de12@google.com/
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Fixes: a8f354284304 ("bcachefs: bch2_print_string_as_lines()")
Cc: <stable@vger.kernel.org> # v6.7+
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Brian Foster <bfoster@redhat.com>
Cc: linux-bcachefs@vger.kernel.org
Cc: Petr Mladek <pmladek@suse.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v2: Dont trylock, drop console_lock entirely
---
 fs/bcachefs/util.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/bcachefs/util.c b/fs/bcachefs/util.c
index de331dec2a99..dc891563d502 100644
--- a/fs/bcachefs/util.c
+++ b/fs/bcachefs/util.c
@@ -8,7 +8,6 @@
 
 #include <linux/bio.h>
 #include <linux/blkdev.h>
-#include <linux/console.h>
 #include <linux/ctype.h>
 #include <linux/debugfs.h>
 #include <linux/freezer.h>
@@ -261,7 +260,6 @@ void bch2_print_string_as_lines(const char *prefix, const char *lines)
 		return;
 	}
 
-	console_lock();
 	while (1) {
 		p = strchrnul(lines, '\n');
 		printk("%s%.*s\n", prefix, (int) (p - lines), lines);
@@ -269,7 +267,6 @@ void bch2_print_string_as_lines(const char *prefix, const char *lines)
 			break;
 		lines = p + 1;
 	}
-	console_unlock();
 }
 
 int bch2_save_backtrace(bch_stacktrace *stack, struct task_struct *task, unsigned skipnr,
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] bcachefs: no console_lock in bch2_print_string_as_lines
  2024-07-10 13:03   ` [PATCH] bcachefs: no console_lock " Daniel Vetter
@ 2024-07-10 14:01     ` John Ogness
  2024-07-10 14:13     ` John Ogness
  1 sibling, 0 replies; 11+ messages in thread
From: John Ogness @ 2024-07-10 14:01 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: Intel Graphics Development, Daniel Vetter,
	syzbot+6cebc1af246fe020a2f0, Daniel Vetter, stable,
	Kent Overstreet, Brian Foster, linux-bcachefs, Petr Mladek,
	Steven Rostedt, Sergey Senozhatsky

On 2024-07-10, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> console_lock is the outermost subsystem lock for a lot of subsystems,
> which means get/put_user must nest within. Which means it cannot be
> acquired somewhere deeply nested in other locks, and most definitely
> not while holding fs locks potentially needed to resolve faults.
>
> console_trylock is the best we can do here. But John pointed out on a
> previous version that this is futile:
>
> "Using the console lock here at all is wrong. The console lock does not
> prevent other CPUs from calling printk() and inserting lines in between.
>
> "There is no way to guarantee a contiguous ringbuffer block using
> multiple printk() calls.
>
> "The console_lock usage should be removed."
>
> https://lore.kernel.org/lkml/87frsh33xp.fsf@jogness.linutronix.de/
>
> Do that.
>
> Reported-by: syzbot+6cebc1af246fe020a2f0@syzkaller.appspotmail.com
> References: https://lore.kernel.org/dri-devel/00000000000026c1ff061cd0de12@google.com/
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Fixes: a8f354284304 ("bcachefs: bch2_print_string_as_lines()")

Reviewed-by: John Ogness <john.ogness@linutronix.de>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bcachefs: no console_lock in bch2_print_string_as_lines
  2024-07-10 13:03   ` [PATCH] bcachefs: no console_lock " Daniel Vetter
  2024-07-10 14:01     ` John Ogness
@ 2024-07-10 14:13     ` John Ogness
  2024-07-15  8:45       ` Daniel Vetter
  1 sibling, 1 reply; 11+ messages in thread
From: John Ogness @ 2024-07-10 14:13 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: Intel Graphics Development, Daniel Vetter,
	syzbot+6cebc1af246fe020a2f0, Daniel Vetter, stable,
	Kent Overstreet, Brian Foster, linux-bcachefs, Petr Mladek,
	Steven Rostedt, Sergey Senozhatsky

On 2024-07-10, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> console_lock is the outermost subsystem lock for a lot of subsystems,
> which means get/put_user must nest within. Which means it cannot be
> acquired somewhere deeply nested in other locks, and most definitely
> not while holding fs locks potentially needed to resolve faults.
>
> console_trylock is the best we can do here. But John pointed out on a
> previous version that this is futile:
>
> "Using the console lock here at all is wrong. The console lock does not
> prevent other CPUs from calling printk() and inserting lines in between.
>
> "There is no way to guarantee a contiguous ringbuffer block using
> multiple printk() calls.
>
> "The console_lock usage should be removed."
>
> https://lore.kernel.org/lkml/87frsh33xp.fsf@jogness.linutronix.de/
>
> Do that.

Note that there is more of this incorrect usage of console lock in:

fs/bcachefs/debug.c:bch2_btree_verify_replica()

fs/bcachefs/bset.c:bch2_dump_btree_node()

from commit 1c6fdbd8f246("bcachefs: Initial commit")

... and its parent bcache:

drivers/md/bcache/debug.c:bch_btree_verify()

drivers/md/bcache/bset.c:bch_dump_bucket()

from commit cafe56359144("bcache: A block layer cache")

These should also be removed. Although Kent should verify that the
console lock is not providing some sort of necessary side-effect
synchronization.

John Ogness

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bcachefs: no console_lock in bch2_print_string_as_lines
  2024-07-10 14:13     ` John Ogness
@ 2024-07-15  8:45       ` Daniel Vetter
  0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2024-07-15  8:45 UTC (permalink / raw)
  To: John Ogness
  Cc: Daniel Vetter, DRI Development, LKML, Intel Graphics Development,
	syzbot+6cebc1af246fe020a2f0, Daniel Vetter, stable,
	Kent Overstreet, Brian Foster, linux-bcachefs, Petr Mladek,
	Steven Rostedt, Sergey Senozhatsky

On Wed, Jul 10, 2024 at 04:19:53PM +0206, John Ogness wrote:
> On 2024-07-10, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > console_lock is the outermost subsystem lock for a lot of subsystems,
> > which means get/put_user must nest within. Which means it cannot be
> > acquired somewhere deeply nested in other locks, and most definitely
> > not while holding fs locks potentially needed to resolve faults.
> >
> > console_trylock is the best we can do here. But John pointed out on a
> > previous version that this is futile:
> >
> > "Using the console lock here at all is wrong. The console lock does not
> > prevent other CPUs from calling printk() and inserting lines in between.
> >
> > "There is no way to guarantee a contiguous ringbuffer block using
> > multiple printk() calls.
> >
> > "The console_lock usage should be removed."
> >
> > https://lore.kernel.org/lkml/87frsh33xp.fsf@jogness.linutronix.de/
> >
> > Do that.
> 
> Note that there is more of this incorrect usage of console lock in:
> 
> fs/bcachefs/debug.c:bch2_btree_verify_replica()
> 
> fs/bcachefs/bset.c:bch2_dump_btree_node()
> 
> from commit 1c6fdbd8f246("bcachefs: Initial commit")
> 
> ... and its parent bcache:
> 
> drivers/md/bcache/debug.c:bch_btree_verify()
> 
> drivers/md/bcache/bset.c:bch_dump_bucket()
> 
> from commit cafe56359144("bcache: A block layer cache")
> 
> These should also be removed. Although Kent should verify that the
> console lock is not providing some sort of necessary side-effect
> synchronization.

I'll take a look, at least some of them seem doable to audit without deep
bcachefs understanding. Thanks for pointing them out, I should have looked
a bit more at git grep ...
-Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming
  2024-07-10 12:40     ` Christian König
@ 2024-07-15 10:08       ` Daniel Vetter
  0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2024-07-15 10:08 UTC (permalink / raw)
  To: Christian König
  Cc: Daniel Vetter, DRI Development, LKML, Intel Graphics Development,
	Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Sumit Semwal, linux-media, linaro-mm-sig

On Wed, Jul 10, 2024 at 02:40:04PM +0200, Christian König wrote:
> Am 10.07.24 um 13:58 schrieb Daniel Vetter:
> > On Wed, 10 Jul 2024 at 13:39, Christian König <christian.koenig@amd.com> wrote:
> > > Am 10.07.24 um 11:31 schrieb Daniel Vetter:
> > > > We already teach lockdep that dma_resv nests within drm_modeset_lock,
> > > > but there's a lot more: All drm kms ioctl rely on being able to
> > > > put/get_user while holding modeset locks, so we really need a
> > > > might_fault in there too to complete the picture. Add it.
> > > Mhm, lockdep should be able to deduce that when there might be faults
> > > under the dma_resv lock there might also be faults under the
> > > drm_modeset_lock.
> > You're not allowed to take a fault under dma_resv, because drivers
> > might need to take that lock to handle faults. So unfortunately in our
> > combined lockdep priming, there really seems to be no chain yet that
> > teaches about faults possibly happening while holding
> > drm_modeset_lock.
> 
> Ah, of course! You are right, it was just the other way around.

Applied to drm-misc-next, thanks for your review.
-Sima

> 
> Thanks,
> Christian.
> 
> > -Sima
> > 
> > > > Motivated by a syzbot report that blew up on bcachefs doing an
> > > > unconditional console_lock way deep in the locking hierarchy, and
> > > > lockdep only noticing the depency loop in a drm ioctl instead of much
> > > > earlier. This annotation will make sure such issues have a much harder
> > > > time escaping.
> > > > 
> > > > References: https://lore.kernel.org/dri-devel/00000000000073db8b061cd43496@google.com/
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Maxime Ripard <mripard@kernel.org>
> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > > > Cc: "Christian König" <christian.koenig@amd.com>
> > > > Cc: linux-media@vger.kernel.org
> > > > Cc: linaro-mm-sig@lists.linaro.org
> > > On the other hand pointing it out explicitly doesn't hurts us at all, so
> > > Reviewed-by: Christian König <christian.koenig@amd.com>.
> > > 
> > > Regards,
> > > Christian.
> > > 
> > > > ---
> > > >    drivers/gpu/drm/drm_mode_config.c | 2 ++
> > > >    1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > > index 568972258222..37d2e0a4ef4b 100644
> > > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > > @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev)
> > > >                if (ret == -EDEADLK)
> > > >                        ret = drm_modeset_backoff(&modeset_ctx);
> > > > 
> > > > +             might_fault();
> > > > +
> > > >                ww_acquire_init(&resv_ctx, &reservation_ww_class);
> > > >                ret = dma_resv_lock(&resv, &resv_ctx);
> > > >                if (ret == -EDEADLK)
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-07-15 10:08 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-10  9:31 [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming Daniel Vetter
2024-07-10  9:31 ` [PATCH 2/2] bcachefs: only console_trylock in bch2_print_string_as_lines Daniel Vetter
2024-07-10 11:02   ` John Ogness
2024-07-10 13:03   ` [PATCH] bcachefs: no console_lock " Daniel Vetter
2024-07-10 14:01     ` John Ogness
2024-07-10 14:13     ` John Ogness
2024-07-15  8:45       ` Daniel Vetter
2024-07-10 11:38 ` [PATCH 1/2] drm: Add might_fault to drm_modeset_lock priming Christian König
2024-07-10 11:58   ` Daniel Vetter
2024-07-10 12:40     ` Christian König
2024-07-15 10:08       ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox