* [PATCH 0/2] fcntl: fix potential deadlocks @ 2021-07-02 9:18 Desmond Cheong Zhi Xi 2021-07-02 9:18 ` [PATCH 1/2] fcntl: fix potential deadlocks for &fown_struct.lock Desmond Cheong Zhi Xi 2021-07-02 9:18 ` [PATCH 2/2] fcntl: fix potential deadlock for &fasync_struct.fa_lock Desmond Cheong Zhi Xi 0 siblings, 2 replies; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-07-02 9:18 UTC (permalink / raw) To: jlayton, bfields, viro Cc: Desmond Cheong Zhi Xi, linux-fsdevel, linux-kernel, skhan, gregkh, linux-kernel-mentees Hi, 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 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 | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] fcntl: fix potential deadlocks for &fown_struct.lock 2021-07-02 9:18 [PATCH 0/2] fcntl: fix potential deadlocks Desmond Cheong Zhi Xi @ 2021-07-02 9:18 ` Desmond Cheong Zhi Xi 2021-07-02 11:44 ` Jeff Layton 2021-07-02 9:18 ` [PATCH 2/2] fcntl: fix potential deadlock for &fasync_struct.fa_lock Desmond Cheong Zhi Xi 1 sibling, 1 reply; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-07-02 9:18 UTC (permalink / raw) To: jlayton, bfields, viro Cc: Desmond Cheong Zhi Xi, linux-fsdevel, linux-kernel, skhan, gregkh, linux-kernel-mentees, syzbot+e6d5398a02c516ce5e70 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. Reported-and-tested-by: syzbot+e6d5398a02c516ce5e70@syzkaller.appspotmail.com Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> --- fs/fcntl.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/fs/fcntl.c b/fs/fcntl.c index dfc72f15be7f..cf9e81dfa615 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -150,7 +150,8 @@ void f_delown(struct file *filp) pid_t f_getown(struct file *filp) { pid_t pid = 0; - read_lock(&filp->f_owner.lock); + + 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 +159,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 +209,7 @@ 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); + 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 +232,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 +250,10 @@ static int f_getowner_uids(struct file *filp, unsigned long arg) uid_t src[2]; int err; - read_lock(&filp->f_owner.lock); + 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] fcntl: fix potential deadlocks for &fown_struct.lock 2021-07-02 9:18 ` [PATCH 1/2] fcntl: fix potential deadlocks for &fown_struct.lock Desmond Cheong Zhi Xi @ 2021-07-02 11:44 ` Jeff Layton 2021-07-02 13:55 ` Desmond Cheong Zhi Xi 0 siblings, 1 reply; 7+ messages in thread From: Jeff Layton @ 2021-07-02 11:44 UTC (permalink / raw) To: Desmond Cheong Zhi Xi, bfields, viro Cc: linux-fsdevel, linux-kernel, skhan, gregkh, linux-kernel-mentees, syzbot+e6d5398a02c516ce5e70 On Fri, 2021-07-02 at 17:18 +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. > Patches look reasonable overall, but why does this one use read_lock_irq and the other one use read_lock_irqsave? Don't we need to *_irqsasve in both patches? > Reported-and-tested-by: syzbot+e6d5398a02c516ce5e70@syzkaller.appspotmail.com > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> > --- > fs/fcntl.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/fs/fcntl.c b/fs/fcntl.c > index dfc72f15be7f..cf9e81dfa615 100644 > --- a/fs/fcntl.c > +++ b/fs/fcntl.c > @@ -150,7 +150,8 @@ void f_delown(struct file *filp) > pid_t f_getown(struct file *filp) > { > pid_t pid = 0; > - read_lock(&filp->f_owner.lock); > + > + 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 +159,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 +209,7 @@ 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); > + 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 +232,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 +250,10 @@ static int f_getowner_uids(struct file *filp, unsigned long arg) > uid_t src[2]; > int err; > > - read_lock(&filp->f_owner.lock); > + 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]); -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] fcntl: fix potential deadlocks for &fown_struct.lock 2021-07-02 11:44 ` Jeff Layton @ 2021-07-02 13:55 ` Desmond Cheong Zhi Xi 2021-07-02 14:27 ` Jeff Layton 0 siblings, 1 reply; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-07-02 13:55 UTC (permalink / raw) To: Jeff Layton, bfields, viro Cc: linux-fsdevel, linux-kernel, skhan, gregkh, linux-kernel-mentees, syzbot+e6d5398a02c516ce5e70 On 2/7/21 7:44 pm, Jeff Layton wrote: > On Fri, 2021-07-02 at 17:18 +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. >> > > Patches look reasonable overall, but why does this one use read_lock_irq > and the other one use read_lock_irqsave? Don't we need to *_irqsasve in > both patches? > > My thinking was that 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() For both paths, it doesn't seem that interrupts are disabled, so I used the *irq variants. But of course, I might be very mistaken on this, and I'd be happy to make the change to *_irqsave. Also, on further inspection, if these calls should be changed to *_irqsave, then I believe the call to write_lock_irq in f_modown (called from do_fcntl() --> f_setown() --> __f_setown() --> f_modown()) should also be changed to *_irqsave. There's also a call to write_lock_irq(&fa->fa_lock) in fasync_remove_entry and fasync_insert_entry. Whether these should be changed as well isn't as clear to me, but since it's safe to do, perhaps it makes sense to use *_irqsave for them too. Thoughts? >> Reported-and-tested-by: syzbot+e6d5398a02c516ce5e70@syzkaller.appspotmail.com >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> >> --- >> fs/fcntl.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/fs/fcntl.c b/fs/fcntl.c >> index dfc72f15be7f..cf9e81dfa615 100644 >> --- a/fs/fcntl.c >> +++ b/fs/fcntl.c >> @@ -150,7 +150,8 @@ void f_delown(struct file *filp) >> pid_t f_getown(struct file *filp) >> { >> pid_t pid = 0; >> - read_lock(&filp->f_owner.lock); >> + >> + 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 +159,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 +209,7 @@ 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); >> + 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 +232,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 +250,10 @@ static int f_getowner_uids(struct file *filp, unsigned long arg) >> uid_t src[2]; >> int err; >> >> - read_lock(&filp->f_owner.lock); >> + 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]); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] fcntl: fix potential deadlocks for &fown_struct.lock 2021-07-02 13:55 ` Desmond Cheong Zhi Xi @ 2021-07-02 14:27 ` Jeff Layton 2021-07-02 15:41 ` Desmond Cheong Zhi Xi 0 siblings, 1 reply; 7+ messages in thread From: Jeff Layton @ 2021-07-02 14:27 UTC (permalink / raw) To: Desmond Cheong Zhi Xi, bfields, viro Cc: linux-fsdevel, linux-kernel, skhan, gregkh, linux-kernel-mentees, syzbot+e6d5398a02c516ce5e70 On Fri, 2021-07-02 at 21:55 +0800, Desmond Cheong Zhi Xi wrote: > On 2/7/21 7:44 pm, Jeff Layton wrote: > > On Fri, 2021-07-02 at 17:18 +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. > > > > > > > Patches look reasonable overall, but why does this one use read_lock_irq > > and the other one use read_lock_irqsave? Don't we need to *_irqsasve in > > both patches? > > > > > > My thinking was that 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() > > For both paths, it doesn't seem that interrupts are disabled, so I used > the *irq variants. > > But of course, I might be very mistaken on this, and I'd be happy to > make the change to *_irqsave. > > Also, on further inspection, if these calls should be changed to > *_irqsave, then I believe the call to write_lock_irq in f_modown (called > from do_fcntl() --> f_setown() --> __f_setown() --> f_modown()) should > also be changed to *_irqsave. > > There's also a call to write_lock_irq(&fa->fa_lock) in > fasync_remove_entry and fasync_insert_entry. Whether these should be > changed as well isn't as clear to me, but since it's safe to do, perhaps > it makes sense to use *_irqsave for them too. Thoughts? > I think your reasoning is probably valid here and we don't need to save/restore. It wasn't obvious to me until you pointed it out though. It might be worth a comment, or maybe even this at the top of both functions: WARN_ON_ONCE(irqs_disabled()); I'll pick these into linux-next soon and plan to merge them for v5.15. Let me know if you think they need to go in sooner. > > > Reported-and-tested-by: syzbot+e6d5398a02c516ce5e70@syzkaller.appspotmail.com > > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> > > > --- > > > fs/fcntl.c | 13 +++++++------ > > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > > > diff --git a/fs/fcntl.c b/fs/fcntl.c > > > index dfc72f15be7f..cf9e81dfa615 100644 > > > --- a/fs/fcntl.c > > > +++ b/fs/fcntl.c > > > @@ -150,7 +150,8 @@ void f_delown(struct file *filp) > > > pid_t f_getown(struct file *filp) > > > { > > > pid_t pid = 0; > > > - read_lock(&filp->f_owner.lock); > > > + > > > + 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 +159,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 +209,7 @@ 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); > > > + 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 +232,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 +250,10 @@ static int f_getowner_uids(struct file *filp, unsigned long arg) > > > uid_t src[2]; > > > int err; > > > > > > - read_lock(&filp->f_owner.lock); > > > + 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]); > > > -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] fcntl: fix potential deadlocks for &fown_struct.lock 2021-07-02 14:27 ` Jeff Layton @ 2021-07-02 15:41 ` Desmond Cheong Zhi Xi 0 siblings, 0 replies; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-07-02 15:41 UTC (permalink / raw) To: Jeff Layton, bfields, viro Cc: linux-fsdevel, linux-kernel, skhan, gregkh, linux-kernel-mentees, syzbot+e6d5398a02c516ce5e70 On 2/7/21 10:27 pm, Jeff Layton wrote: > On Fri, 2021-07-02 at 21:55 +0800, Desmond Cheong Zhi Xi wrote: >> On 2/7/21 7:44 pm, Jeff Layton wrote: >>> On Fri, 2021-07-02 at 17:18 +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. >>>> >>> >>> Patches look reasonable overall, but why does this one use read_lock_irq >>> and the other one use read_lock_irqsave? Don't we need to *_irqsasve in >>> both patches? >>> >>> >> >> My thinking was that 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() >> >> For both paths, it doesn't seem that interrupts are disabled, so I used >> the *irq variants. >> >> But of course, I might be very mistaken on this, and I'd be happy to >> make the change to *_irqsave. >> >> Also, on further inspection, if these calls should be changed to >> *_irqsave, then I believe the call to write_lock_irq in f_modown (called >> from do_fcntl() --> f_setown() --> __f_setown() --> f_modown()) should >> also be changed to *_irqsave. >> >> There's also a call to write_lock_irq(&fa->fa_lock) in >> fasync_remove_entry and fasync_insert_entry. Whether these should be >> changed as well isn't as clear to me, but since it's safe to do, perhaps >> it makes sense to use *_irqsave for them too. Thoughts? >> > > > I think your reasoning is probably valid here and we don't need to > save/restore. It wasn't obvious to me until you pointed it out though. > It might be worth a comment, or maybe even this at the top of both > functions: > > WARN_ON_ONCE(irqs_disabled()); > Adding the WARN_ON_ONCE makes sense. I'll test it with Syzbot then prepare a v2 series. > I'll pick these into linux-next soon and plan to merge them for v5.15. > Let me know if you think they need to go in sooner. > > Sounds good to me. Thanks for the feedback, Jeff. >>>> Reported-and-tested-by: syzbot+e6d5398a02c516ce5e70@syzkaller.appspotmail.com >>>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> >>>> --- >>>> fs/fcntl.c | 13 +++++++------ >>>> 1 file changed, 7 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/fs/fcntl.c b/fs/fcntl.c >>>> index dfc72f15be7f..cf9e81dfa615 100644 >>>> --- a/fs/fcntl.c >>>> +++ b/fs/fcntl.c >>>> @@ -150,7 +150,8 @@ void f_delown(struct file *filp) >>>> pid_t f_getown(struct file *filp) >>>> { >>>> pid_t pid = 0; >>>> - read_lock(&filp->f_owner.lock); >>>> + >>>> + 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 +159,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 +209,7 @@ 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); >>>> + 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 +232,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 +250,10 @@ static int f_getowner_uids(struct file *filp, unsigned long arg) >>>> uid_t src[2]; >>>> int err; >>>> >>>> - read_lock(&filp->f_owner.lock); >>>> + 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]); >>> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] fcntl: fix potential deadlock for &fasync_struct.fa_lock 2021-07-02 9:18 [PATCH 0/2] fcntl: fix potential deadlocks Desmond Cheong Zhi Xi 2021-07-02 9:18 ` [PATCH 1/2] fcntl: fix potential deadlocks for &fown_struct.lock Desmond Cheong Zhi Xi @ 2021-07-02 9:18 ` Desmond Cheong Zhi Xi 1 sibling, 0 replies; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-07-02 9:18 UTC (permalink / raw) To: jlayton, bfields, viro Cc: Desmond Cheong Zhi Xi, linux-fsdevel, linux-kernel, skhan, gregkh, linux-kernel-mentees 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 <desmondcheongzx@gmail.com> --- fs/fcntl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fs/fcntl.c b/fs/fcntl.c index cf9e81dfa615..887db4918a89 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -1004,13 +1004,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 @@ -1019,7 +1020,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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-02 15:41 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-02 9:18 [PATCH 0/2] fcntl: fix potential deadlocks Desmond Cheong Zhi Xi 2021-07-02 9:18 ` [PATCH 1/2] fcntl: fix potential deadlocks for &fown_struct.lock Desmond Cheong Zhi Xi 2021-07-02 11:44 ` Jeff Layton 2021-07-02 13:55 ` Desmond Cheong Zhi Xi 2021-07-02 14:27 ` Jeff Layton 2021-07-02 15:41 ` Desmond Cheong Zhi Xi 2021-07-02 9:18 ` [PATCH 2/2] fcntl: fix potential deadlock for &fasync_struct.fa_lock Desmond Cheong Zhi Xi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).