Hi,
Sorry for the delay between v1 and v2, there was an unrelated issue with Syzbot testing.
Syzbot reports a possible irq lock inversion dependency:
https://syzkaller.appspot.com/bug?id=923cfc6c6348963f99886a0176ef11dcc429547b
While investigating this error, I discovered that multiple similar lock inversion scenarios can occur. Hence, this series addresses potential deadlocks for two classes of locks, one in each patch:
1. Fix potential deadlocks for &fown_struct.lock
2. Fix potential deadlock for &fasync_struct.fa_lock
v1 -> v2:
- Added WARN_ON_ONCE(irqs_disabled()) before calls to read_lock_irq, with elaboration in the commit message, as suggested by Jeff Layton.
Best wishes,
Desmond
Desmond Cheong Zhi Xi (2):
fcntl: fix potential deadlocks for &fown_struct.lock
fcntl: fix potential deadlock for &fasync_struct.fa_lock
fs/fcntl.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
--
2.25.1
Syzbot reports a potential deadlock in do_fcntl:
========================================================
WARNING: possible irq lock inversion dependency detected
5.12.0-syzkaller #0 Not tainted
--------------------------------------------------------
syz-executor132/8391 just changed the state of lock:
ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: f_getown_ex fs/fcntl.c:211 [inline]
ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: do_fcntl+0x8b4/0x1200 fs/fcntl.c:395
but this lock was taken by another, HARDIRQ-safe lock in the past:
(&dev->event_lock){-...}-{2:2}
and interrupts could create inverse lock ordering between them.
other info that might help us debug this:
Chain exists of:
&dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&f->f_owner.lock);
local_irq_disable();
lock(&dev->event_lock);
lock(&new->fa_lock);
<Interrupt>
lock(&dev->event_lock);
*** DEADLOCK ***
This happens because there is a lock hierarchy of
&dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
from the following call chain:
input_inject_event():
spin_lock_irqsave(&dev->event_lock,...);
input_handle_event():
input_pass_values():
input_to_handler():
evdev_events():
evdev_pass_values():
spin_lock(&client->buffer_lock);
__pass_event():
kill_fasync():
kill_fasync_rcu():
read_lock(&fa->fa_lock);
send_sigio():
read_lock_irqsave(&fown->lock,...);
However, since &dev->event_lock is HARDIRQ-safe, interrupts have to be
disabled while grabbing &f->f_owner.lock, otherwise we invert the lock
hierarchy.
Hence, we replace calls to read_lock/read_unlock on &f->f_owner.lock,
with read_lock_irq/read_unlock_irq.
Here read_lock_irq/read_unlock_irq should be safe to use because the
functions f_getown_ex and f_getowner_uids are only called from
do_fcntl, and f_getown is only called from do_fnctl and
sock_ioctl. do_fnctl itself is only called from syscalls.
For sock_ioctl, the chain is
compat_sock_ioctl():
compat_sock_ioctl_trans():
sock_ioctl()
And interrupts are not disabled on either path. We assert this
assumption with WARN_ON_ONCE(irqs_disabled()). This check is also
inserted into another use of write_lock_irq in f_modown.
Reported-and-tested-by: [email protected]
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
fs/fcntl.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index dfc72f15be7f..262235e02c4b 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -88,6 +88,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
int force)
{
+ WARN_ON_ONCE(irqs_disabled());
write_lock_irq(&filp->f_owner.lock);
if (force || !filp->f_owner.pid) {
put_pid(filp->f_owner.pid);
@@ -150,7 +151,9 @@ void f_delown(struct file *filp)
pid_t f_getown(struct file *filp)
{
pid_t pid = 0;
- read_lock(&filp->f_owner.lock);
+
+ WARN_ON_ONCE(irqs_disabled());
+ read_lock_irq(&filp->f_owner.lock);
rcu_read_lock();
if (pid_task(filp->f_owner.pid, filp->f_owner.pid_type)) {
pid = pid_vnr(filp->f_owner.pid);
@@ -158,7 +161,7 @@ pid_t f_getown(struct file *filp)
pid = -pid;
}
rcu_read_unlock();
- read_unlock(&filp->f_owner.lock);
+ read_unlock_irq(&filp->f_owner.lock);
return pid;
}
@@ -208,7 +211,8 @@ static int f_getown_ex(struct file *filp, unsigned long arg)
struct f_owner_ex owner = {};
int ret = 0;
- read_lock(&filp->f_owner.lock);
+ WARN_ON_ONCE(irqs_disabled());
+ read_lock_irq(&filp->f_owner.lock);
rcu_read_lock();
if (pid_task(filp->f_owner.pid, filp->f_owner.pid_type))
owner.pid = pid_vnr(filp->f_owner.pid);
@@ -231,7 +235,7 @@ static int f_getown_ex(struct file *filp, unsigned long arg)
ret = -EINVAL;
break;
}
- read_unlock(&filp->f_owner.lock);
+ read_unlock_irq(&filp->f_owner.lock);
if (!ret) {
ret = copy_to_user(owner_p, &owner, sizeof(owner));
@@ -249,10 +253,11 @@ static int f_getowner_uids(struct file *filp, unsigned long arg)
uid_t src[2];
int err;
- read_lock(&filp->f_owner.lock);
+ WARN_ON_ONCE(irqs_disabled());
+ read_lock_irq(&filp->f_owner.lock);
src[0] = from_kuid(user_ns, filp->f_owner.uid);
src[1] = from_kuid(user_ns, filp->f_owner.euid);
- read_unlock(&filp->f_owner.lock);
+ read_unlock_irq(&filp->f_owner.lock);
err = put_user(src[0], &dst[0]);
err |= put_user(src[1], &dst[1]);
--
2.25.1
There is an existing lock hierarchy of
&dev->event_lock --> &fasync_struct.fa_lock --> &f->f_owner.lock
from the following call chain:
input_inject_event():
spin_lock_irqsave(&dev->event_lock,...);
input_handle_event():
input_pass_values():
input_to_handler():
evdev_events():
evdev_pass_values():
spin_lock(&client->buffer_lock);
__pass_event():
kill_fasync():
kill_fasync_rcu():
read_lock(&fa->fa_lock);
send_sigio():
read_lock_irqsave(&fown->lock,...);
&dev->event_lock is HARDIRQ-safe, so interrupts have to be disabled
while grabbing &fasync_struct.fa_lock, otherwise we invert the lock
hierarchy. However, since kill_fasync which calls kill_fasync_rcu is
an exported symbol, it may not necessarily be called with interrupts
disabled.
As kill_fasync_rcu may be called with interrupts disabled (for
example, in the call chain above), we replace calls to
read_lock/read_unlock on &fasync_struct.fa_lock in kill_fasync_rcu
with read_lock_irqsave/read_unlock_irqrestore.
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
fs/fcntl.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 262235e02c4b..fd9c895d704c 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1008,13 +1008,14 @@ static void kill_fasync_rcu(struct fasync_struct *fa, int sig, int band)
{
while (fa) {
struct fown_struct *fown;
+ unsigned long flags;
if (fa->magic != FASYNC_MAGIC) {
printk(KERN_ERR "kill_fasync: bad magic number in "
"fasync_struct!\n");
return;
}
- read_lock(&fa->fa_lock);
+ read_lock_irqsave(&fa->fa_lock, flags);
if (fa->fa_file) {
fown = &fa->fa_file->f_owner;
/* Don't send SIGURG to processes which have not set a
@@ -1023,7 +1024,7 @@ static void kill_fasync_rcu(struct fasync_struct *fa, int sig, int band)
if (!(sig == SIGURG && fown->signum == 0))
send_sigio(fown, fa->fa_fd, band);
}
- read_unlock(&fa->fa_lock);
+ read_unlock_irqrestore(&fa->fa_lock, flags);
fa = rcu_dereference(fa->fa_next);
}
}
--
2.25.1
On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> Syzbot reports a potential deadlock in do_fcntl:
>
> ========================================================
> WARNING: possible irq lock inversion dependency detected
> 5.12.0-syzkaller #0 Not tainted
> --------------------------------------------------------
> syz-executor132/8391 just changed the state of lock:
> ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: f_getown_ex fs/fcntl.c:211 [inline]
> ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: do_fcntl+0x8b4/0x1200 fs/fcntl.c:395
> but this lock was taken by another, HARDIRQ-safe lock in the past:
> (&dev->event_lock){-...}-{2:2}
>
> and interrupts could create inverse lock ordering between them.
>
> other info that might help us debug this:
> Chain exists of:
> &dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
>
> Possible interrupt unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&f->f_owner.lock);
> local_irq_disable();
> lock(&dev->event_lock);
> lock(&new->fa_lock);
> <Interrupt>
> lock(&dev->event_lock);
>
> *** DEADLOCK ***
>
> This happens because there is a lock hierarchy of
> &dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
> from the following call chain:
>
> input_inject_event():
> spin_lock_irqsave(&dev->event_lock,...);
> input_handle_event():
> input_pass_values():
> input_to_handler():
> evdev_events():
> evdev_pass_values():
> spin_lock(&client->buffer_lock);
> __pass_event():
> kill_fasync():
> kill_fasync_rcu():
> read_lock(&fa->fa_lock);
> send_sigio():
> read_lock_irqsave(&fown->lock,...);
>
> However, since &dev->event_lock is HARDIRQ-safe, interrupts have to be
> disabled while grabbing &f->f_owner.lock, otherwise we invert the lock
> hierarchy.
>
> Hence, we replace calls to read_lock/read_unlock on &f->f_owner.lock,
> with read_lock_irq/read_unlock_irq.
>
> Here read_lock_irq/read_unlock_irq should be safe to use because the
> functions f_getown_ex and f_getowner_uids are only called from
> do_fcntl, and f_getown is only called from do_fnctl and
> sock_ioctl. do_fnctl itself is only called from syscalls.
>
> For sock_ioctl, the chain is
> compat_sock_ioctl():
> compat_sock_ioctl_trans():
> sock_ioctl()
>
> And interrupts are not disabled on either path. We assert this
> assumption with WARN_ON_ONCE(irqs_disabled()). This check is also
> inserted into another use of write_lock_irq in f_modown.
>
> Reported-and-tested-by: [email protected]
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> ---
> fs/fcntl.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index dfc72f15be7f..262235e02c4b 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -88,6 +88,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
> static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> int force)
> {
> + WARN_ON_ONCE(irqs_disabled());
If this triggers, you just rebooted the box :(
Please never do this, either properly handle the problem and return an
error, or do not check for this. It is not any type of "fix" at all,
and at most, a debugging aid while you work on the root problem.
thanks,
greg k-h
On 7/7/21 2:05 pm, Greg KH wrote:
> On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
>> Syzbot reports a potential deadlock in do_fcntl:
>>
>> ========================================================
>> WARNING: possible irq lock inversion dependency detected
>> 5.12.0-syzkaller #0 Not tainted
>> --------------------------------------------------------
>> syz-executor132/8391 just changed the state of lock:
>> ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: f_getown_ex fs/fcntl.c:211 [inline]
>> ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: do_fcntl+0x8b4/0x1200 fs/fcntl.c:395
>> but this lock was taken by another, HARDIRQ-safe lock in the past:
>> (&dev->event_lock){-...}-{2:2}
>>
>> and interrupts could create inverse lock ordering between them.
>>
>> other info that might help us debug this:
>> Chain exists of:
>> &dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
>>
>> Possible interrupt unsafe locking scenario:
>>
>> CPU0 CPU1
>> ---- ----
>> lock(&f->f_owner.lock);
>> local_irq_disable();
>> lock(&dev->event_lock);
>> lock(&new->fa_lock);
>> <Interrupt>
>> lock(&dev->event_lock);
>>
>> *** DEADLOCK ***
>>
>> This happens because there is a lock hierarchy of
>> &dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
>> from the following call chain:
>>
>> input_inject_event():
>> spin_lock_irqsave(&dev->event_lock,...);
>> input_handle_event():
>> input_pass_values():
>> input_to_handler():
>> evdev_events():
>> evdev_pass_values():
>> spin_lock(&client->buffer_lock);
>> __pass_event():
>> kill_fasync():
>> kill_fasync_rcu():
>> read_lock(&fa->fa_lock);
>> send_sigio():
>> read_lock_irqsave(&fown->lock,...);
>>
>> However, since &dev->event_lock is HARDIRQ-safe, interrupts have to be
>> disabled while grabbing &f->f_owner.lock, otherwise we invert the lock
>> hierarchy.
>>
>> Hence, we replace calls to read_lock/read_unlock on &f->f_owner.lock,
>> with read_lock_irq/read_unlock_irq.
>>
>> Here read_lock_irq/read_unlock_irq should be safe to use because the
>> functions f_getown_ex and f_getowner_uids are only called from
>> do_fcntl, and f_getown is only called from do_fnctl and
>> sock_ioctl. do_fnctl itself is only called from syscalls.
>>
>> For sock_ioctl, the chain is
>> compat_sock_ioctl():
>> compat_sock_ioctl_trans():
>> sock_ioctl()
>>
>> And interrupts are not disabled on either path. We assert this
>> assumption with WARN_ON_ONCE(irqs_disabled()). This check is also
>> inserted into another use of write_lock_irq in f_modown.
>>
>> Reported-and-tested-by: [email protected]
>> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
>> ---
>> fs/fcntl.c | 17 +++++++++++------
>> 1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/fcntl.c b/fs/fcntl.c
>> index dfc72f15be7f..262235e02c4b 100644
>> --- a/fs/fcntl.c
>> +++ b/fs/fcntl.c
>> @@ -88,6 +88,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
>> static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
>> int force)
>> {
>> + WARN_ON_ONCE(irqs_disabled());
>
> If this triggers, you just rebooted the box :(
>
> Please never do this, either properly handle the problem and return an
> error, or do not check for this. It is not any type of "fix" at all,
> and at most, a debugging aid while you work on the root problem.
>
> thanks,
>
> greg k-h
>
Hi Greg,
Thanks for the feedback. My bad, I was under the impression that
WARN_ON_ONCE could be used to document assumptions for other developers,
but I'll stick to using it for debugging in the future.
I think then in this case it would be best to keep the reasoning for why
the *_irq() locks are safe to use in the commit message. I'll update the
patch accordingly.
Best wishes,
Desmond
On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > Syzbot reports a potential deadlock in do_fcntl:
> >
> > ========================================================
> > WARNING: possible irq lock inversion dependency detected
> > 5.12.0-syzkaller #0 Not tainted
> > --------------------------------------------------------
> > syz-executor132/8391 just changed the state of lock:
> > ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: f_getown_ex fs/fcntl.c:211 [inline]
> > ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: do_fcntl+0x8b4/0x1200 fs/fcntl.c:395
> > but this lock was taken by another, HARDIRQ-safe lock in the past:
> > (&dev->event_lock){-...}-{2:2}
> >
> > and interrupts could create inverse lock ordering between them.
> >
> > other info that might help us debug this:
> > Chain exists of:
> > &dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
> >
> > Possible interrupt unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(&f->f_owner.lock);
> > local_irq_disable();
> > lock(&dev->event_lock);
> > lock(&new->fa_lock);
> > <Interrupt>
> > lock(&dev->event_lock);
> >
> > *** DEADLOCK ***
> >
> > This happens because there is a lock hierarchy of
> > &dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
> > from the following call chain:
> >
> > input_inject_event():
> > spin_lock_irqsave(&dev->event_lock,...);
> > input_handle_event():
> > input_pass_values():
> > input_to_handler():
> > evdev_events():
> > evdev_pass_values():
> > spin_lock(&client->buffer_lock);
> > __pass_event():
> > kill_fasync():
> > kill_fasync_rcu():
> > read_lock(&fa->fa_lock);
> > send_sigio():
> > read_lock_irqsave(&fown->lock,...);
> >
> > However, since &dev->event_lock is HARDIRQ-safe, interrupts have to be
> > disabled while grabbing &f->f_owner.lock, otherwise we invert the lock
> > hierarchy.
> >
> > Hence, we replace calls to read_lock/read_unlock on &f->f_owner.lock,
> > with read_lock_irq/read_unlock_irq.
> >
> > Here read_lock_irq/read_unlock_irq should be safe to use because the
> > functions f_getown_ex and f_getowner_uids are only called from
> > do_fcntl, and f_getown is only called from do_fnctl and
> > sock_ioctl. do_fnctl itself is only called from syscalls.
> >
> > For sock_ioctl, the chain is
> > compat_sock_ioctl():
> > compat_sock_ioctl_trans():
> > sock_ioctl()
> >
> > And interrupts are not disabled on either path. We assert this
> > assumption with WARN_ON_ONCE(irqs_disabled()). This check is also
> > inserted into another use of write_lock_irq in f_modown.
> >
> > Reported-and-tested-by: [email protected]
> > Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> > ---
> > fs/fcntl.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > index dfc72f15be7f..262235e02c4b 100644
> > --- a/fs/fcntl.c
> > +++ b/fs/fcntl.c
> > @@ -88,6 +88,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
> > static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> > int force)
> > {
> > + WARN_ON_ONCE(irqs_disabled());
>
> If this triggers, you just rebooted the box :(
>
> Please never do this, either properly handle the problem and return an
> error, or do not check for this. It is not any type of "fix" at all,
> and at most, a debugging aid while you work on the root problem.
>
> thanks,
>
> greg k-h
Wait, what? Why would testing for irqs being disabled and throwing a
WARN_ON in that case crash the box?
--
Jeff Layton <[email protected]>
On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > Syzbot reports a potential deadlock in do_fcntl:
> > >
> > > ========================================================
> > > WARNING: possible irq lock inversion dependency detected
> > > 5.12.0-syzkaller #0 Not tainted
> > > --------------------------------------------------------
> > > syz-executor132/8391 just changed the state of lock:
> > > ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: f_getown_ex fs/fcntl.c:211 [inline]
> > > ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: do_fcntl+0x8b4/0x1200 fs/fcntl.c:395
> > > but this lock was taken by another, HARDIRQ-safe lock in the past:
> > > (&dev->event_lock){-...}-{2:2}
> > >
> > > and interrupts could create inverse lock ordering between them.
> > >
> > > other info that might help us debug this:
> > > Chain exists of:
> > > &dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
> > >
> > > Possible interrupt unsafe locking scenario:
> > >
> > > CPU0 CPU1
> > > ---- ----
> > > lock(&f->f_owner.lock);
> > > local_irq_disable();
> > > lock(&dev->event_lock);
> > > lock(&new->fa_lock);
> > > <Interrupt>
> > > lock(&dev->event_lock);
> > >
> > > *** DEADLOCK ***
> > >
> > > This happens because there is a lock hierarchy of
> > > &dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
> > > from the following call chain:
> > >
> > > input_inject_event():
> > > spin_lock_irqsave(&dev->event_lock,...);
> > > input_handle_event():
> > > input_pass_values():
> > > input_to_handler():
> > > evdev_events():
> > > evdev_pass_values():
> > > spin_lock(&client->buffer_lock);
> > > __pass_event():
> > > kill_fasync():
> > > kill_fasync_rcu():
> > > read_lock(&fa->fa_lock);
> > > send_sigio():
> > > read_lock_irqsave(&fown->lock,...);
> > >
> > > However, since &dev->event_lock is HARDIRQ-safe, interrupts have to be
> > > disabled while grabbing &f->f_owner.lock, otherwise we invert the lock
> > > hierarchy.
> > >
> > > Hence, we replace calls to read_lock/read_unlock on &f->f_owner.lock,
> > > with read_lock_irq/read_unlock_irq.
> > >
> > > Here read_lock_irq/read_unlock_irq should be safe to use because the
> > > functions f_getown_ex and f_getowner_uids are only called from
> > > do_fcntl, and f_getown is only called from do_fnctl and
> > > sock_ioctl. do_fnctl itself is only called from syscalls.
> > >
> > > For sock_ioctl, the chain is
> > > compat_sock_ioctl():
> > > compat_sock_ioctl_trans():
> > > sock_ioctl()
> > >
> > > And interrupts are not disabled on either path. We assert this
> > > assumption with WARN_ON_ONCE(irqs_disabled()). This check is also
> > > inserted into another use of write_lock_irq in f_modown.
> > >
> > > Reported-and-tested-by: [email protected]
> > > Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> > > ---
> > > fs/fcntl.c | 17 +++++++++++------
> > > 1 file changed, 11 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > > index dfc72f15be7f..262235e02c4b 100644
> > > --- a/fs/fcntl.c
> > > +++ b/fs/fcntl.c
> > > @@ -88,6 +88,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
> > > static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> > > int force)
> > > {
> > > + WARN_ON_ONCE(irqs_disabled());
> >
> > If this triggers, you just rebooted the box :(
> >
> > Please never do this, either properly handle the problem and return an
> > error, or do not check for this. It is not any type of "fix" at all,
> > and at most, a debugging aid while you work on the root problem.
> >
> > thanks,
> >
> > greg k-h
>
> Wait, what? Why would testing for irqs being disabled and throwing a
> WARN_ON in that case crash the box?
If panic-on-warn is enabled, which is a common setting for systems these
days.
On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > Syzbot reports a potential deadlock in do_fcntl:
> > > >
> > > > ========================================================
> > > > WARNING: possible irq lock inversion dependency detected
> > > > 5.12.0-syzkaller #0 Not tainted
> > > > --------------------------------------------------------
> > > > syz-executor132/8391 just changed the state of lock:
> > > > ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: f_getown_ex fs/fcntl.c:211 [inline]
> > > > ffff888015967bf8 (&f->f_owner.lock){.+..}-{2:2}, at: do_fcntl+0x8b4/0x1200 fs/fcntl.c:395
> > > > but this lock was taken by another, HARDIRQ-safe lock in the past:
> > > > (&dev->event_lock){-...}-{2:2}
> > > >
> > > > and interrupts could create inverse lock ordering between them.
> > > >
> > > > other info that might help us debug this:
> > > > Chain exists of:
> > > > &dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
> > > >
> > > > Possible interrupt unsafe locking scenario:
> > > >
> > > > CPU0 CPU1
> > > > ---- ----
> > > > lock(&f->f_owner.lock);
> > > > local_irq_disable();
> > > > lock(&dev->event_lock);
> > > > lock(&new->fa_lock);
> > > > <Interrupt>
> > > > lock(&dev->event_lock);
> > > >
> > > > *** DEADLOCK ***
> > > >
> > > > This happens because there is a lock hierarchy of
> > > > &dev->event_lock --> &new->fa_lock --> &f->f_owner.lock
> > > > from the following call chain:
> > > >
> > > > input_inject_event():
> > > > spin_lock_irqsave(&dev->event_lock,...);
> > > > input_handle_event():
> > > > input_pass_values():
> > > > input_to_handler():
> > > > evdev_events():
> > > > evdev_pass_values():
> > > > spin_lock(&client->buffer_lock);
> > > > __pass_event():
> > > > kill_fasync():
> > > > kill_fasync_rcu():
> > > > read_lock(&fa->fa_lock);
> > > > send_sigio():
> > > > read_lock_irqsave(&fown->lock,...);
> > > >
> > > > However, since &dev->event_lock is HARDIRQ-safe, interrupts have to be
> > > > disabled while grabbing &f->f_owner.lock, otherwise we invert the lock
> > > > hierarchy.
> > > >
> > > > Hence, we replace calls to read_lock/read_unlock on &f->f_owner.lock,
> > > > with read_lock_irq/read_unlock_irq.
> > > >
> > > > Here read_lock_irq/read_unlock_irq should be safe to use because the
> > > > functions f_getown_ex and f_getowner_uids are only called from
> > > > do_fcntl, and f_getown is only called from do_fnctl and
> > > > sock_ioctl. do_fnctl itself is only called from syscalls.
> > > >
> > > > For sock_ioctl, the chain is
> > > > compat_sock_ioctl():
> > > > compat_sock_ioctl_trans():
> > > > sock_ioctl()
> > > >
> > > > And interrupts are not disabled on either path. We assert this
> > > > assumption with WARN_ON_ONCE(irqs_disabled()). This check is also
> > > > inserted into another use of write_lock_irq in f_modown.
> > > >
> > > > Reported-and-tested-by: [email protected]
> > > > Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> > > > ---
> > > > fs/fcntl.c | 17 +++++++++++------
> > > > 1 file changed, 11 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > > > index dfc72f15be7f..262235e02c4b 100644
> > > > --- a/fs/fcntl.c
> > > > +++ b/fs/fcntl.c
> > > > @@ -88,6 +88,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
> > > > static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> > > > int force)
> > > > {
> > > > + WARN_ON_ONCE(irqs_disabled());
> > >
> > > If this triggers, you just rebooted the box :(
> > >
> > > Please never do this, either properly handle the problem and return an
> > > error, or do not check for this. It is not any type of "fix" at all,
> > > and at most, a debugging aid while you work on the root problem.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > Wait, what? Why would testing for irqs being disabled and throwing a
> > WARN_ON in that case crash the box?
>
> If panic-on-warn is enabled, which is a common setting for systems these
> days.
Ok, that makes some sense.
Thanks,
--
Jeff Layton <[email protected]>
On Wed, Jul 07, 2021 at 09:51:29AM -0400, J. Bruce Fields wrote:
> On Wed, Jul 07, 2021 at 07:40:47AM -0400, Jeff Layton wrote:
> > On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> > > On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > > > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > > > + WARN_ON_ONCE(irqs_disabled());
> > > > >
> > > > > If this triggers, you just rebooted the box :(
> > > > >
> > > > > Please never do this, either properly handle the problem and return an
> > > > > error, or do not check for this. It is not any type of "fix" at all,
> > > > > and at most, a debugging aid while you work on the root problem.
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > > >
> > > > Wait, what? Why would testing for irqs being disabled and throwing a
> > > > WARN_ON in that case crash the box?
> > >
> > > If panic-on-warn is enabled, which is a common setting for systems these
> > > days.
> >
> > Ok, that makes some sense.
>
> Wait, I don't get it.
>
> How are we supposed to decide when to use WARN, when to use BUG, and
> when to panic? Do we really want to treat them all as equivalent? And
> who exactly is turning on panic-on-warn?
You never use WARN or BUG, unless the system is so messed up that you
can not possibly recover from the issue. Don't be lazy, handle the
error properly and report it upwards.
thanks,
greg k-h
On Wed, Jul 07, 2021 at 05:06:45PM +0200, Greg KH wrote:
> On Wed, Jul 07, 2021 at 09:51:29AM -0400, J. Bruce Fields wrote:
> > On Wed, Jul 07, 2021 at 07:40:47AM -0400, Jeff Layton wrote:
> > > On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> > > > On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > > > > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > > > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > > > > + WARN_ON_ONCE(irqs_disabled());
> > > > > >
> > > > > > If this triggers, you just rebooted the box :(
> > > > > >
> > > > > > Please never do this, either properly handle the problem and return an
> > > > > > error, or do not check for this. It is not any type of "fix" at all,
> > > > > > and at most, a debugging aid while you work on the root problem.
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > greg k-h
> > > > >
> > > > > Wait, what? Why would testing for irqs being disabled and throwing a
> > > > > WARN_ON in that case crash the box?
> > > >
> > > > If panic-on-warn is enabled, which is a common setting for systems these
> > > > days.
> > >
> > > Ok, that makes some sense.
> >
> > Wait, I don't get it.
> >
> > How are we supposed to decide when to use WARN, when to use BUG, and
> > when to panic? Do we really want to treat them all as equivalent? And
> > who exactly is turning on panic-on-warn?
>
> You never use WARN or BUG, unless the system is so messed up that you
> can not possibly recover from the issue.
I've heard similar advice for BUG before, but this is the first I've
heard it for WARN. Do we have any guidelines for how to choose between
WARN and BUG?
--b.
On Wed, Jul 07, 2021 at 07:40:47AM -0400, Jeff Layton wrote:
> On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> > On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > > + WARN_ON_ONCE(irqs_disabled());
> > > >
> > > > If this triggers, you just rebooted the box :(
> > > >
> > > > Please never do this, either properly handle the problem and return an
> > > > error, or do not check for this. It is not any type of "fix" at all,
> > > > and at most, a debugging aid while you work on the root problem.
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > Wait, what? Why would testing for irqs being disabled and throwing a
> > > WARN_ON in that case crash the box?
> >
> > If panic-on-warn is enabled, which is a common setting for systems these
> > days.
>
> Ok, that makes some sense.
Wait, I don't get it.
How are we supposed to decide when to use WARN, when to use BUG, and
when to panic? Do we really want to treat them all as equivalent? And
who exactly is turning on panic-on-warn?
--b.
On Wed, Jul 07, 2021 at 05:31:06PM +0200, Greg KH wrote:
> On Wed, Jul 07, 2021 at 11:19:36AM -0400, J. Bruce Fields wrote:
> > On Wed, Jul 07, 2021 at 05:06:45PM +0200, Greg KH wrote:
> > > On Wed, Jul 07, 2021 at 09:51:29AM -0400, J. Bruce Fields wrote:
> > > > On Wed, Jul 07, 2021 at 07:40:47AM -0400, Jeff Layton wrote:
> > > > > On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> > > > > > On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > > > > > > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > > > > > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > > > > > > + WARN_ON_ONCE(irqs_disabled());
> > > > > > > >
> > > > > > > > If this triggers, you just rebooted the box :(
> > > > > > > >
> > > > > > > > Please never do this, either properly handle the problem and return an
> > > > > > > > error, or do not check for this. It is not any type of "fix" at all,
> > > > > > > > and at most, a debugging aid while you work on the root problem.
> > > > > > > >
> > > > > > > > thanks,
> > > > > > > >
> > > > > > > > greg k-h
> > > > > > >
> > > > > > > Wait, what? Why would testing for irqs being disabled and throwing a
> > > > > > > WARN_ON in that case crash the box?
> > > > > >
> > > > > > If panic-on-warn is enabled, which is a common setting for systems these
> > > > > > days.
> > > > >
> > > > > Ok, that makes some sense.
> > > >
> > > > Wait, I don't get it.
> > > >
> > > > How are we supposed to decide when to use WARN, when to use BUG, and
> > > > when to panic? Do we really want to treat them all as equivalent? And
> > > > who exactly is turning on panic-on-warn?
> > >
> > > You never use WARN or BUG, unless the system is so messed up that you
> > > can not possibly recover from the issue.
> >
> > I've heard similar advice for BUG before, but this is the first I've
> > heard it for WARN. Do we have any guidelines for how to choose between
> > WARN and BUG?
>
> Never use either :)
I can't tell if you're kidding. Is there some plan to remove them?
There are definitely cases where I've been able to resolve a problem
more quickly because I got a backtrace from a WARN.
--b.
On Wed, Jul 07, 2021 at 11:34:17AM -0400, J. Bruce Fields wrote:
> On Wed, Jul 07, 2021 at 05:31:06PM +0200, Greg KH wrote:
> > On Wed, Jul 07, 2021 at 11:19:36AM -0400, J. Bruce Fields wrote:
> > > On Wed, Jul 07, 2021 at 05:06:45PM +0200, Greg KH wrote:
> > > > On Wed, Jul 07, 2021 at 09:51:29AM -0400, J. Bruce Fields wrote:
> > > > > On Wed, Jul 07, 2021 at 07:40:47AM -0400, Jeff Layton wrote:
> > > > > > On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> > > > > > > On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > > > > > > > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > > > > > > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > > > > > > > + WARN_ON_ONCE(irqs_disabled());
> > > > > > > > >
> > > > > > > > > If this triggers, you just rebooted the box :(
> > > > > > > > >
> > > > > > > > > Please never do this, either properly handle the problem and return an
> > > > > > > > > error, or do not check for this. It is not any type of "fix" at all,
> > > > > > > > > and at most, a debugging aid while you work on the root problem.
> > > > > > > > >
> > > > > > > > > thanks,
> > > > > > > > >
> > > > > > > > > greg k-h
> > > > > > > >
> > > > > > > > Wait, what? Why would testing for irqs being disabled and throwing a
> > > > > > > > WARN_ON in that case crash the box?
> > > > > > >
> > > > > > > If panic-on-warn is enabled, which is a common setting for systems these
> > > > > > > days.
> > > > > >
> > > > > > Ok, that makes some sense.
> > > > >
> > > > > Wait, I don't get it.
> > > > >
> > > > > How are we supposed to decide when to use WARN, when to use BUG, and
> > > > > when to panic? Do we really want to treat them all as equivalent? And
> > > > > who exactly is turning on panic-on-warn?
> > > >
> > > > You never use WARN or BUG, unless the system is so messed up that you
> > > > can not possibly recover from the issue.
> > >
> > > I've heard similar advice for BUG before, but this is the first I've
> > > heard it for WARN. Do we have any guidelines for how to choose between
> > > WARN and BUG?
> >
> > Never use either :)
>
> I can't tell if you're kidding.
I am not.
> Is there some plan to remove them?
Over time, yes. And any WARN that userspace can ever hit should be
removed today.
> There are definitely cases where I've been able to resolve a problem
> more quickly because I got a backtrace from a WARN.
If you want a backtrace, ask for that, recover from the error, and move
on. Do not allow userspace to reboot a machine for no good reason as
again, panic-on-warn is a common setting that people use now.
This is what all of the syzbot work has been doing, it triggers things
that cause WARN() to be hit and so we have to fix them.
thanks,
greg k-h
On Wed, Jul 07, 2021 at 12:18:45PM -0400, Jeff Layton wrote:
> On Wed, 2021-07-07 at 17:46 +0200, Greg KH wrote:
> > On Wed, Jul 07, 2021 at 11:34:17AM -0400, J. Bruce Fields wrote:
> > > On Wed, Jul 07, 2021 at 05:31:06PM +0200, Greg KH wrote:
> > > > On Wed, Jul 07, 2021 at 11:19:36AM -0400, J. Bruce Fields wrote:
> > > > > On Wed, Jul 07, 2021 at 05:06:45PM +0200, Greg KH wrote:
> > > > > > On Wed, Jul 07, 2021 at 09:51:29AM -0400, J. Bruce Fields wrote:
> > > > > > > On Wed, Jul 07, 2021 at 07:40:47AM -0400, Jeff Layton wrote:
> > > > > > > > On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> > > > > > > > > On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > > > > > > > > > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > > > > > > > > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > > > > > > > > > + WARN_ON_ONCE(irqs_disabled());
> > > > > > > > > > >
> > > > > > > > > > > If this triggers, you just rebooted the box :(
> > > > > > > > > > >
> > > > > > > > > > > Please never do this, either properly handle the problem and return an
> > > > > > > > > > > error, or do not check for this. It is not any type of "fix" at all,
> > > > > > > > > > > and at most, a debugging aid while you work on the root problem.
> > > > > > > > > > >
> > > > > > > > > > > thanks,
> > > > > > > > > > >
> > > > > > > > > > > greg k-h
> > > > > > > > > >
> > > > > > > > > > Wait, what? Why would testing for irqs being disabled and throwing a
> > > > > > > > > > WARN_ON in that case crash the box?
> > > > > > > > >
> > > > > > > > > If panic-on-warn is enabled, which is a common setting for systems these
> > > > > > > > > days.
> > > > > > > >
> > > > > > > > Ok, that makes some sense.
> > > > > > >
> > > > > > > Wait, I don't get it.
> > > > > > >
> > > > > > > How are we supposed to decide when to use WARN, when to use BUG, and
> > > > > > > when to panic? Do we really want to treat them all as equivalent? And
> > > > > > > who exactly is turning on panic-on-warn?
> > > > > >
> > > > > > You never use WARN or BUG, unless the system is so messed up that you
> > > > > > can not possibly recover from the issue.
> > > > >
> > > > > I've heard similar advice for BUG before, but this is the first I've
> > > > > heard it for WARN. Do we have any guidelines for how to choose between
> > > > > WARN and BUG?
> > > >
> > > > Never use either :)
> > >
> > > I can't tell if you're kidding.
> >
> > I am not.
> >
> > > Is there some plan to remove them?
> >
> > Over time, yes. And any WARN that userspace can ever hit should be
> > removed today.
> >
> > > There are definitely cases where I've been able to resolve a problem
> > > more quickly because I got a backtrace from a WARN.
> >
> > If you want a backtrace, ask for that, recover from the error, and move
> > on. Do not allow userspace to reboot a machine for no good reason as
> > again, panic-on-warn is a common setting that people use now.
> >
> > This is what all of the syzbot work has been doing, it triggers things
> > that cause WARN() to be hit and so we have to fix them.
> >
>
> This seems really draconian. Clearly we do want to fix things that show
> a WARN (otherwise we wouldn't bother warning about it), but I don't
> think that's a reason to completely avoid them. My understanding has
> always been:
>
> BUG: for when you reach some condition where the kernel (probably) can't
> carry on
>
> WARN: for when you reach some condition that is problematic but where
> the machine can probably soldier on.
>
> Over the last several years, I've changed a lot of BUGs into WARNs to
> avoid crashing the box unnecessarily. If someone is setting
> panic_on_warn, then aren't they just getting what they asked for?
>
> While I don't feel that strongly about this particular WARN in this
> patch, it seems like a reasonable thing to do. If someone calls these
> functions with IRQs disabled, then they might end up with some subtle
> problems that could be hard to detect otherwise.
Don't we already have a debugging option that would catch this?
config DEBUG_IRQFLAGS
bool "Debug IRQ flag manipulation"
help
Enables checks for potentially unsafe enabling or disabling of
interrupts, such as calling raw_local_irq_restore() when interrupts
are enabled.
so I think this particular warn is unnecessary.
But I also disagree with Greg. Normal users aren't setting panic-on-warn.
Various build bots are setting panic-on-warn -- and they should -- because
we shouldn't be able to trigger these kinds of warnings from userspace.
Those are bugs that should be fixed. But there's no reason to shy away
from using a WARN when it's the right thing to do.
On Wed, Jul 07, 2021 at 11:19:36AM -0400, J. Bruce Fields wrote:
> On Wed, Jul 07, 2021 at 05:06:45PM +0200, Greg KH wrote:
> > On Wed, Jul 07, 2021 at 09:51:29AM -0400, J. Bruce Fields wrote:
> > > On Wed, Jul 07, 2021 at 07:40:47AM -0400, Jeff Layton wrote:
> > > > On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> > > > > On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > > > > > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > > > > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > > > > > + WARN_ON_ONCE(irqs_disabled());
> > > > > > >
> > > > > > > If this triggers, you just rebooted the box :(
> > > > > > >
> > > > > > > Please never do this, either properly handle the problem and return an
> > > > > > > error, or do not check for this. It is not any type of "fix" at all,
> > > > > > > and at most, a debugging aid while you work on the root problem.
> > > > > > >
> > > > > > > thanks,
> > > > > > >
> > > > > > > greg k-h
> > > > > >
> > > > > > Wait, what? Why would testing for irqs being disabled and throwing a
> > > > > > WARN_ON in that case crash the box?
> > > > >
> > > > > If panic-on-warn is enabled, which is a common setting for systems these
> > > > > days.
> > > >
> > > > Ok, that makes some sense.
> > >
> > > Wait, I don't get it.
> > >
> > > How are we supposed to decide when to use WARN, when to use BUG, and
> > > when to panic? Do we really want to treat them all as equivalent? And
> > > who exactly is turning on panic-on-warn?
> >
> > You never use WARN or BUG, unless the system is so messed up that you
> > can not possibly recover from the issue.
>
> I've heard similar advice for BUG before, but this is the first I've
> heard it for WARN. Do we have any guidelines for how to choose between
> WARN and BUG?
Never use either :)
On Wed, 2021-07-07 at 17:46 +0200, Greg KH wrote:
> On Wed, Jul 07, 2021 at 11:34:17AM -0400, J. Bruce Fields wrote:
> > On Wed, Jul 07, 2021 at 05:31:06PM +0200, Greg KH wrote:
> > > On Wed, Jul 07, 2021 at 11:19:36AM -0400, J. Bruce Fields wrote:
> > > > On Wed, Jul 07, 2021 at 05:06:45PM +0200, Greg KH wrote:
> > > > > On Wed, Jul 07, 2021 at 09:51:29AM -0400, J. Bruce Fields wrote:
> > > > > > On Wed, Jul 07, 2021 at 07:40:47AM -0400, Jeff Layton wrote:
> > > > > > > On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> > > > > > > > On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > > > > > > > > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > > > > > > > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > > > > > > > > + WARN_ON_ONCE(irqs_disabled());
> > > > > > > > > >
> > > > > > > > > > If this triggers, you just rebooted the box :(
> > > > > > > > > >
> > > > > > > > > > Please never do this, either properly handle the problem and return an
> > > > > > > > > > error, or do not check for this. It is not any type of "fix" at all,
> > > > > > > > > > and at most, a debugging aid while you work on the root problem.
> > > > > > > > > >
> > > > > > > > > > thanks,
> > > > > > > > > >
> > > > > > > > > > greg k-h
> > > > > > > > >
> > > > > > > > > Wait, what? Why would testing for irqs being disabled and throwing a
> > > > > > > > > WARN_ON in that case crash the box?
> > > > > > > >
> > > > > > > > If panic-on-warn is enabled, which is a common setting for systems these
> > > > > > > > days.
> > > > > > >
> > > > > > > Ok, that makes some sense.
> > > > > >
> > > > > > Wait, I don't get it.
> > > > > >
> > > > > > How are we supposed to decide when to use WARN, when to use BUG, and
> > > > > > when to panic? Do we really want to treat them all as equivalent? And
> > > > > > who exactly is turning on panic-on-warn?
> > > > >
> > > > > You never use WARN or BUG, unless the system is so messed up that you
> > > > > can not possibly recover from the issue.
> > > >
> > > > I've heard similar advice for BUG before, but this is the first I've
> > > > heard it for WARN. Do we have any guidelines for how to choose between
> > > > WARN and BUG?
> > >
> > > Never use either :)
> >
> > I can't tell if you're kidding.
>
> I am not.
>
> > Is there some plan to remove them?
>
> Over time, yes. And any WARN that userspace can ever hit should be
> removed today.
>
> > There are definitely cases where I've been able to resolve a problem
> > more quickly because I got a backtrace from a WARN.
>
> If you want a backtrace, ask for that, recover from the error, and move
> on. Do not allow userspace to reboot a machine for no good reason as
> again, panic-on-warn is a common setting that people use now.
>
> This is what all of the syzbot work has been doing, it triggers things
> that cause WARN() to be hit and so we have to fix them.
>
This seems really draconian. Clearly we do want to fix things that show
a WARN (otherwise we wouldn't bother warning about it), but I don't
think that's a reason to completely avoid them. My understanding has
always been:
BUG: for when you reach some condition where the kernel (probably) can't
carry on
WARN: for when you reach some condition that is problematic but where
the machine can probably soldier on.
Over the last several years, I've changed a lot of BUGs into WARNs to
avoid crashing the box unnecessarily. If someone is setting
panic_on_warn, then aren't they just getting what they asked for?
While I don't feel that strongly about this particular WARN in this
patch, it seems like a reasonable thing to do. If someone calls these
functions with IRQs disabled, then they might end up with some subtle
problems that could be hard to detect otherwise.
--
Jeff Layton <[email protected]>
On Wed, Jul 07, 2021 at 05:25:19PM +0100, Matthew Wilcox wrote:
> On Wed, Jul 07, 2021 at 12:18:45PM -0400, Jeff Layton wrote:
> > On Wed, 2021-07-07 at 17:46 +0200, Greg KH wrote:
> > > On Wed, Jul 07, 2021 at 11:34:17AM -0400, J. Bruce Fields wrote:
> > > > On Wed, Jul 07, 2021 at 05:31:06PM +0200, Greg KH wrote:
> > > > > On Wed, Jul 07, 2021 at 11:19:36AM -0400, J. Bruce Fields wrote:
> > > > > > On Wed, Jul 07, 2021 at 05:06:45PM +0200, Greg KH wrote:
> > > > > > > On Wed, Jul 07, 2021 at 09:51:29AM -0400, J. Bruce Fields wrote:
> > > > > > > > On Wed, Jul 07, 2021 at 07:40:47AM -0400, Jeff Layton wrote:
> > > > > > > > > On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> > > > > > > > > > On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > > > > > > > > > > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > > > > > > > > > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > > > > > > > > > > + WARN_ON_ONCE(irqs_disabled());
> > > > > > > > > > > >
> > > > > > > > > > > > If this triggers, you just rebooted the box :(
> > > > > > > > > > > >
> > > > > > > > > > > > Please never do this, either properly handle the problem and return an
> > > > > > > > > > > > error, or do not check for this. It is not any type of "fix" at all,
> > > > > > > > > > > > and at most, a debugging aid while you work on the root problem.
> > > > > > > > > > > >
> > > > > > > > > > > > thanks,
> > > > > > > > > > > >
> > > > > > > > > > > > greg k-h
> > > > > > > > > > >
> > > > > > > > > > > Wait, what? Why would testing for irqs being disabled and throwing a
> > > > > > > > > > > WARN_ON in that case crash the box?
> > > > > > > > > >
> > > > > > > > > > If panic-on-warn is enabled, which is a common setting for systems these
> > > > > > > > > > days.
> > > > > > > > >
> > > > > > > > > Ok, that makes some sense.
> > > > > > > >
> > > > > > > > Wait, I don't get it.
> > > > > > > >
> > > > > > > > How are we supposed to decide when to use WARN, when to use BUG, and
> > > > > > > > when to panic? Do we really want to treat them all as equivalent? And
> > > > > > > > who exactly is turning on panic-on-warn?
> > > > > > >
> > > > > > > You never use WARN or BUG, unless the system is so messed up that you
> > > > > > > can not possibly recover from the issue.
> > > > > >
> > > > > > I've heard similar advice for BUG before, but this is the first I've
> > > > > > heard it for WARN. Do we have any guidelines for how to choose between
> > > > > > WARN and BUG?
> > > > >
> > > > > Never use either :)
> > > >
> > > > I can't tell if you're kidding.
> > >
> > > I am not.
> > >
> > > > Is there some plan to remove them?
> > >
> > > Over time, yes. And any WARN that userspace can ever hit should be
> > > removed today.
> > >
> > > > There are definitely cases where I've been able to resolve a problem
> > > > more quickly because I got a backtrace from a WARN.
> > >
> > > If you want a backtrace, ask for that, recover from the error, and move
> > > on. Do not allow userspace to reboot a machine for no good reason as
> > > again, panic-on-warn is a common setting that people use now.
> > >
> > > This is what all of the syzbot work has been doing, it triggers things
> > > that cause WARN() to be hit and so we have to fix them.
> > >
> >
> > This seems really draconian. Clearly we do want to fix things that show
> > a WARN (otherwise we wouldn't bother warning about it), but I don't
> > think that's a reason to completely avoid them. My understanding has
> > always been:
> >
> > BUG: for when you reach some condition where the kernel (probably) can't
> > carry on
> >
> > WARN: for when you reach some condition that is problematic but where
> > the machine can probably soldier on.
> >
> > Over the last several years, I've changed a lot of BUGs into WARNs to
> > avoid crashing the box unnecessarily. If someone is setting
> > panic_on_warn, then aren't they just getting what they asked for?
> >
> > While I don't feel that strongly about this particular WARN in this
> > patch, it seems like a reasonable thing to do. If someone calls these
> > functions with IRQs disabled, then they might end up with some subtle
> > problems that could be hard to detect otherwise.
>
> Don't we already have a debugging option that would catch this?
>
> config DEBUG_IRQFLAGS
> bool "Debug IRQ flag manipulation"
> help
> Enables checks for potentially unsafe enabling or disabling of
> interrupts, such as calling raw_local_irq_restore() when interrupts
> are enabled.
>
> so I think this particular warn is unnecessary.
>
> But I also disagree with Greg. Normal users aren't setting panic-on-warn.
> Various build bots are setting panic-on-warn -- and they should -- because
> we shouldn't be able to trigger these kinds of warnings from userspace.
> Those are bugs that should be fixed. But there's no reason to shy away
> from using a WARN when it's the right thing to do.
Yes, WARN is the right choice for signaling a kernel bug that is "recoverable",
e.g. by returning an error or simply ignoring it. WARN is the wrong choice only
when the condition is user-triggerable, e.g. via invalid inputs passed to system
calls. I don't understand why Greg is advocating against all use of WARN; that
would make it harder for kernel bugs to be found and reported. Users of
panic_on_warn (which are usually test systems) *want* the kernel to panic if an
assertion fails -- that's the whole point of it. I'm not sure why we are still
having this discussion, as the differences between and valid uses for WARN and
BUG were documented in include/asm-generic/bug.h a long time ago...
- Eric
On Wed, 2021-07-07 at 17:25 +0100, Matthew Wilcox wrote:
> On Wed, Jul 07, 2021 at 12:18:45PM -0400, Jeff Layton wrote:
> > On Wed, 2021-07-07 at 17:46 +0200, Greg KH wrote:
> > > On Wed, Jul 07, 2021 at 11:34:17AM -0400, J. Bruce Fields wrote:
> > > > On Wed, Jul 07, 2021 at 05:31:06PM +0200, Greg KH wrote:
> > > > > On Wed, Jul 07, 2021 at 11:19:36AM -0400, J. Bruce Fields wrote:
> > > > > > On Wed, Jul 07, 2021 at 05:06:45PM +0200, Greg KH wrote:
> > > > > > > On Wed, Jul 07, 2021 at 09:51:29AM -0400, J. Bruce Fields wrote:
> > > > > > > > On Wed, Jul 07, 2021 at 07:40:47AM -0400, Jeff Layton wrote:
> > > > > > > > > On Wed, 2021-07-07 at 12:51 +0200, Greg KH wrote:
> > > > > > > > > > On Wed, Jul 07, 2021 at 06:44:42AM -0400, Jeff Layton wrote:
> > > > > > > > > > > On Wed, 2021-07-07 at 08:05 +0200, Greg KH wrote:
> > > > > > > > > > > > On Wed, Jul 07, 2021 at 10:35:47AM +0800, Desmond Cheong Zhi Xi wrote:
> > > > > > > > > > > > > + WARN_ON_ONCE(irqs_disabled());
> > > > > > > > > > > >
> > > > > > > > > > > > If this triggers, you just rebooted the box :(
> > > > > > > > > > > >
> > > > > > > > > > > > Please never do this, either properly handle the problem and return an
> > > > > > > > > > > > error, or do not check for this. It is not any type of "fix" at all,
> > > > > > > > > > > > and at most, a debugging aid while you work on the root problem.
> > > > > > > > > > > >
> > > > > > > > > > > > thanks,
> > > > > > > > > > > >
> > > > > > > > > > > > greg k-h
> > > > > > > > > > >
> > > > > > > > > > > Wait, what? Why would testing for irqs being disabled and throwing a
> > > > > > > > > > > WARN_ON in that case crash the box?
> > > > > > > > > >
> > > > > > > > > > If panic-on-warn is enabled, which is a common setting for systems these
> > > > > > > > > > days.
> > > > > > > > >
> > > > > > > > > Ok, that makes some sense.
> > > > > > > >
> > > > > > > > Wait, I don't get it.
> > > > > > > >
> > > > > > > > How are we supposed to decide when to use WARN, when to use BUG, and
> > > > > > > > when to panic? Do we really want to treat them all as equivalent? And
> > > > > > > > who exactly is turning on panic-on-warn?
> > > > > > >
> > > > > > > You never use WARN or BUG, unless the system is so messed up that you
> > > > > > > can not possibly recover from the issue.
> > > > > >
> > > > > > I've heard similar advice for BUG before, but this is the first I've
> > > > > > heard it for WARN. Do we have any guidelines for how to choose between
> > > > > > WARN and BUG?
> > > > >
> > > > > Never use either :)
> > > >
> > > > I can't tell if you're kidding.
> > >
> > > I am not.
> > >
> > > > Is there some plan to remove them?
> > >
> > > Over time, yes. And any WARN that userspace can ever hit should be
> > > removed today.
> > >
> > > > There are definitely cases where I've been able to resolve a problem
> > > > more quickly because I got a backtrace from a WARN.
> > >
> > > If you want a backtrace, ask for that, recover from the error, and move
> > > on. Do not allow userspace to reboot a machine for no good reason as
> > > again, panic-on-warn is a common setting that people use now.
> > >
> > > This is what all of the syzbot work has been doing, it triggers things
> > > that cause WARN() to be hit and so we have to fix them.
> > >
> >
> > This seems really draconian. Clearly we do want to fix things that show
> > a WARN (otherwise we wouldn't bother warning about it), but I don't
> > think that's a reason to completely avoid them. My understanding has
> > always been:
> >
> > BUG: for when you reach some condition where the kernel (probably) can't
> > carry on
> >
> > WARN: for when you reach some condition that is problematic but where
> > the machine can probably soldier on.
> >
> > Over the last several years, I've changed a lot of BUGs into WARNs to
> > avoid crashing the box unnecessarily. If someone is setting
> > panic_on_warn, then aren't they just getting what they asked for?
> >
> > While I don't feel that strongly about this particular WARN in this
> > patch, it seems like a reasonable thing to do. If someone calls these
> > functions with IRQs disabled, then they might end up with some subtle
> > problems that could be hard to detect otherwise.
>
> Don't we already have a debugging option that would catch this?
>
> config DEBUG_IRQFLAGS
> bool "Debug IRQ flag manipulation"
> help
> Enables checks for potentially unsafe enabling or disabling of
> interrupts, such as calling raw_local_irq_restore() when interrupts
> are enabled.
>
> so I think this particular warn is unnecessary.
>
Good to know. I'm just going to leave Desmond's v1 patch (which didn't
have this WARN_ON) in linux-next for now.
> But I also disagree with Greg. Normal users aren't setting panic-on-warn.
> Various build bots are setting panic-on-warn -- and they should -- because
> we shouldn't be able to trigger these kinds of warnings from userspace.
> Those are bugs that should be fixed. But there's no reason to shy away
> from using a WARN when it's the right thing to do.
Agreed.
--
Jeff Layton <[email protected]>