Messages in this thread Patch in this message |  | | | Subject | Re: [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync | | From | (Eric W. Biederman) | | Date | Sat, 06 Feb 2010 23:00:20 -0800 |
| |
Américo Wang <xiyou.wangcong@gmail.com> writes:
> On Sat, Feb 06, 2010 at 10:31:30PM -0800, Linus Torvalds wrote: >> >> >>On Sun, 7 Feb 2010, Tetsuo Handa wrote: >>> >>> Below problem (which was introduced between 2.6.33-rc4 and 2.6.33-rc5) is >>> not yet fixed as of 2.6.33-rc7. >>> "git bisect start v2.6.33-rc5 v2.6.33-rc4" reported that >>> 703625118069f9f8960d356676662d3db5a9d116 tty: fix race in tty_fasync >>> is first bad commit. >> >>Yeah. I think we need to just revert that commit. >> >>Or maybe we could just do the following, rather than revert it outright: >>just get a ref to the 'struct pid' while holding the spinlock, and then >>releasing it after doing the __f_setown() call. > > We already fixed this, a better fix: > > http://lkml.org/lkml/2010/1/26/338 > > I sent a same fix with Greg's.
That fix is present. See below. Why does lockdep still warn? Do we have lockdep bug?
Given that there is the only place we take f_owner.lock for write I don't see how f_owner.lock can be unsafe to be called with irqs disabled.
commit b04da8bfdfbbd79544cab2fadfdc12e87eb01600 Author: Greg Kroah-Hartman <gregkh@suse.de> Date: Tue Jan 26 15:04:02 2010 -0800
fnctl: f_modown should call write_lock_irqsave/restore
Commit 703625118069f9f8960d356676662d3db5a9d116 exposed that f_modown() should call write_lock_irqsave instead of just write_lock_irq so that because a caller could have a spinlock held and it would not be good to renable interrupts.
Cc: Eric W. Biederman <ebiederm@xmission.com> Cc: Al Viro <viro@ZenIV.linux.org.uk> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> Cc: Tavis Ormandy <taviso@google.com> Cc: stable <stable@kernel.org> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
diff --git a/fs/fcntl.c b/fs/fcntl.c index 97e01dc..5ef953e 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -199,7 +199,9 @@ 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) { - write_lock_irq(&filp->f_owner.lock); + unsigned long flags; + + write_lock_irqsave(&filp->f_owner.lock, flags); if (force || !filp->f_owner.pid) { put_pid(filp->f_owner.pid); filp->f_owner.pid = get_pid(pid); @@ -211,7 +213,7 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type, filp->f_owner.euid = cred->euid; } } - write_unlock_irq(&filp->f_owner.lock); + write_unlock_irqrestore(&filp->f_owner.lock, flags); } int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |