* 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60
@ 2013-11-13 11:34 Michael Marineau
2013-11-13 12:39 ` Al Viro
0 siblings, 1 reply; 9+ messages in thread
From: Michael Marineau @ 2013-11-13 11:34 UTC (permalink / raw)
To: Waiman Long; +Cc: linux-kernel, Al Viro
Greetings,
Commit 232d2d60aa5469bb097f55728f65146bd49c1d25 causes intermittent
errors in /proc/*/fd/* where readlink returns "/" instead of the
correct path. This can be reproduced by the script below which copies
the kernel source directory structure while obsessively looking up
directory fds in proc from another process. Reverting
232d2d60aa5469bb097f55728f65146bd49c1d25 after two related commits
48f5ec21d9c67e881ff35343988e290ef5cf933f
1812997720ab90d029548778c55d7315555e1fef fixes the issue.
Cheers,
Mike
#!/usr/bin/python
import os
import subprocess
import tempfile
KERNEL_SOURCE="/home/marineam/git/linux"
TEMP_DIR=tempfile.mkdtemp()
# Copy the directory tree, can skip most of the files
rsync = subprocess.Popen(["rsync", "--exclude=*.*", "-a",
KERNEL_SOURCE, TEMP_DIR])
def check_entry(dir_path, name):
expect = "%s/%s" % (dir_path, name)
if os.path.islink(expect):
return
fd = os.open(expect, os.O_RDONLY)
# It takes some pounding on proc to be likely to hit an error
for i in xrange(100):
got = os.readlink("/proc/self/fd/%d" % fd)
if got != expect:
print "Got %s instead of %s" % (got, expect)
os.close(fd)
try:
for dirpath, dirnames, filenames in os.walk(KERNEL_SOURCE):
for name in dirnames:
check_entry(dirpath, name)
finally:
rsync.wait()
subprocess.call(["rm", "-rf", TEMP_DIR])
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60
2013-11-13 11:34 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60 Michael Marineau
@ 2013-11-13 12:39 ` Al Viro
2013-11-13 14:49 ` Long, Wai Man
2013-11-13 22:51 ` Michael Marineau
0 siblings, 2 replies; 9+ messages in thread
From: Al Viro @ 2013-11-13 12:39 UTC (permalink / raw)
To: Michael Marineau; +Cc: Waiman Long, linux-kernel
On Wed, Nov 13, 2013 at 03:34:13AM -0800, Michael Marineau wrote:
> Greetings,
>
> Commit 232d2d60aa5469bb097f55728f65146bd49c1d25 causes intermittent
> errors in /proc/*/fd/* where readlink returns "/" instead of the
> correct path. This can be reproduced by the script below which copies
> the kernel source directory structure while obsessively looking up
> directory fds in proc from another process. Reverting
> 232d2d60aa5469bb097f55728f65146bd49c1d25 after two related commits
> 48f5ec21d9c67e881ff35343988e290ef5cf933f
> 1812997720ab90d029548778c55d7315555e1fef fixes the issue.
Looking into it... It seems that we are getting to the end of
prepend_path() with non-negative error and bptr == *buffer.
What the...
OK, I see what's going on. We never reinitialize dentry, vfsmount and mnt
if we decide to restart. See if the following helps:
diff --git a/fs/dcache.c b/fs/dcache.c
index ae6ebb8..89f9671 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2881,9 +2881,9 @@ static int prepend_path(const struct path *path,
const struct path *root,
char **buffer, int *buflen)
{
- struct dentry *dentry = path->dentry;
- struct vfsmount *vfsmnt = path->mnt;
- struct mount *mnt = real_mount(vfsmnt);
+ struct dentry *dentry;
+ struct vfsmount *vfsmnt;
+ struct mount *mnt;
int error = 0;
unsigned seq = 0;
char *bptr;
@@ -2893,6 +2893,9 @@ static int prepend_path(const struct path *path,
restart:
bptr = *buffer;
blen = *buflen;
+ dentry = path->dentry;
+ vfsmnt = path->mnt;
+ mnt = real_mount(vfsmnt);
read_seqbegin_or_lock(&rename_lock, &seq);
while (dentry != root->dentry || vfsmnt != root->mnt) {
struct dentry * parent;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60
2013-11-13 12:39 ` Al Viro
@ 2013-11-13 14:49 ` Long, Wai Man
2013-11-13 22:51 ` Michael Marineau
1 sibling, 0 replies; 9+ messages in thread
From: Long, Wai Man @ 2013-11-13 14:49 UTC (permalink / raw)
To: Al Viro, Michael Marineau; +Cc: linux-kernel@vger.kernel.org
Al,
Thank a lot for quickly locating the bug.
-Longman
-----Original Message-----
From: Al Viro [mailto:viro@ftp.linux.org.uk] On Behalf Of Al Viro
Sent: Wednesday, November 13, 2013 7:40 AM
To: Michael Marineau
Cc: Long, Wai Man; linux-kernel@vger.kernel.org
Subject: Re: 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60
On Wed, Nov 13, 2013 at 03:34:13AM -0800, Michael Marineau wrote:
> Greetings,
>
> Commit 232d2d60aa5469bb097f55728f65146bd49c1d25 causes intermittent
> errors in /proc/*/fd/* where readlink returns "/" instead of the
> correct path. This can be reproduced by the script below which copies
> the kernel source directory structure while obsessively looking up
> directory fds in proc from another process. Reverting
> 232d2d60aa5469bb097f55728f65146bd49c1d25 after two related commits
> 48f5ec21d9c67e881ff35343988e290ef5cf933f
> 1812997720ab90d029548778c55d7315555e1fef fixes the issue.
Looking into it... It seems that we are getting to the end of
prepend_path() with non-negative error and bptr == *buffer.
What the...
OK, I see what's going on. We never reinitialize dentry, vfsmount and mnt if we decide to restart. See if the following helps:
diff --git a/fs/dcache.c b/fs/dcache.c
index ae6ebb8..89f9671 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2881,9 +2881,9 @@ static int prepend_path(const struct path *path,
const struct path *root,
char **buffer, int *buflen)
{
- struct dentry *dentry = path->dentry;
- struct vfsmount *vfsmnt = path->mnt;
- struct mount *mnt = real_mount(vfsmnt);
+ struct dentry *dentry;
+ struct vfsmount *vfsmnt;
+ struct mount *mnt;
int error = 0;
unsigned seq = 0;
char *bptr;
@@ -2893,6 +2893,9 @@ static int prepend_path(const struct path *path,
restart:
bptr = *buffer;
blen = *buflen;
+ dentry = path->dentry;
+ vfsmnt = path->mnt;
+ mnt = real_mount(vfsmnt);
read_seqbegin_or_lock(&rename_lock, &seq);
while (dentry != root->dentry || vfsmnt != root->mnt) {
struct dentry * parent;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60
2013-11-13 12:39 ` Al Viro
2013-11-13 14:49 ` Long, Wai Man
@ 2013-11-13 22:51 ` Michael Marineau
2013-11-21 23:01 ` Greg KH
1 sibling, 1 reply; 9+ messages in thread
From: Michael Marineau @ 2013-11-13 22:51 UTC (permalink / raw)
To: Al Viro; +Cc: Waiman Long, linux-kernel
On Wed, Nov 13, 2013 at 4:39 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Nov 13, 2013 at 03:34:13AM -0800, Michael Marineau wrote:
>> Greetings,
>>
>> Commit 232d2d60aa5469bb097f55728f65146bd49c1d25 causes intermittent
>> errors in /proc/*/fd/* where readlink returns "/" instead of the
>> correct path. This can be reproduced by the script below which copies
>> the kernel source directory structure while obsessively looking up
>> directory fds in proc from another process. Reverting
>> 232d2d60aa5469bb097f55728f65146bd49c1d25 after two related commits
>> 48f5ec21d9c67e881ff35343988e290ef5cf933f
>> 1812997720ab90d029548778c55d7315555e1fef fixes the issue.
>
> Looking into it... It seems that we are getting to the end of
> prepend_path() with non-negative error and bptr == *buffer.
> What the...
>
> OK, I see what's going on. We never reinitialize dentry, vfsmount and mnt
> if we decide to restart. See if the following helps:
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index ae6ebb8..89f9671 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2881,9 +2881,9 @@ static int prepend_path(const struct path *path,
> const struct path *root,
> char **buffer, int *buflen)
> {
> - struct dentry *dentry = path->dentry;
> - struct vfsmount *vfsmnt = path->mnt;
> - struct mount *mnt = real_mount(vfsmnt);
> + struct dentry *dentry;
> + struct vfsmount *vfsmnt;
> + struct mount *mnt;
> int error = 0;
> unsigned seq = 0;
> char *bptr;
> @@ -2893,6 +2893,9 @@ static int prepend_path(const struct path *path,
> restart:
> bptr = *buffer;
> blen = *buflen;
> + dentry = path->dentry;
> + vfsmnt = path->mnt;
> + mnt = real_mount(vfsmnt);
> read_seqbegin_or_lock(&rename_lock, &seq);
> while (dentry != root->dentry || vfsmnt != root->mnt) {
> struct dentry * parent;
That appears to do the trick! I've tried my test case against that
patch on both Linus' git tree (as of last night) and the 3.12 release.
I'm now running the long build job that initially stumbled across this
bug now but it looks good so far.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60
2013-11-13 22:51 ` Michael Marineau
@ 2013-11-21 23:01 ` Greg KH
2013-11-21 23:16 ` Michael Marineau
2013-11-21 23:22 ` Al Viro
0 siblings, 2 replies; 9+ messages in thread
From: Greg KH @ 2013-11-21 23:01 UTC (permalink / raw)
To: Michael Marineau, Al Viro; +Cc: Waiman Long, linux-kernel
On Wed, Nov 13, 2013 at 02:51:59PM -0800, Michael Marineau wrote:
> On Wed, Nov 13, 2013 at 4:39 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Wed, Nov 13, 2013 at 03:34:13AM -0800, Michael Marineau wrote:
> >> Greetings,
> >>
> >> Commit 232d2d60aa5469bb097f55728f65146bd49c1d25 causes intermittent
> >> errors in /proc/*/fd/* where readlink returns "/" instead of the
> >> correct path. This can be reproduced by the script below which copies
> >> the kernel source directory structure while obsessively looking up
> >> directory fds in proc from another process. Reverting
> >> 232d2d60aa5469bb097f55728f65146bd49c1d25 after two related commits
> >> 48f5ec21d9c67e881ff35343988e290ef5cf933f
> >> 1812997720ab90d029548778c55d7315555e1fef fixes the issue.
> >
> > Looking into it... It seems that we are getting to the end of
> > prepend_path() with non-negative error and bptr == *buffer.
> > What the...
> >
> > OK, I see what's going on. We never reinitialize dentry, vfsmount and mnt
> > if we decide to restart. See if the following helps:
> >
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index ae6ebb8..89f9671 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -2881,9 +2881,9 @@ static int prepend_path(const struct path *path,
> > const struct path *root,
> > char **buffer, int *buflen)
> > {
> > - struct dentry *dentry = path->dentry;
> > - struct vfsmount *vfsmnt = path->mnt;
> > - struct mount *mnt = real_mount(vfsmnt);
> > + struct dentry *dentry;
> > + struct vfsmount *vfsmnt;
> > + struct mount *mnt;
> > int error = 0;
> > unsigned seq = 0;
> > char *bptr;
> > @@ -2893,6 +2893,9 @@ static int prepend_path(const struct path *path,
> > restart:
> > bptr = *buffer;
> > blen = *buflen;
> > + dentry = path->dentry;
> > + vfsmnt = path->mnt;
> > + mnt = real_mount(vfsmnt);
> > read_seqbegin_or_lock(&rename_lock, &seq);
> > while (dentry != root->dentry || vfsmnt != root->mnt) {
> > struct dentry * parent;
>
> That appears to do the trick! I've tried my test case against that
> patch on both Linus' git tree (as of last night) and the 3.12 release.
> I'm now running the long build job that initially stumbled across this
> bug now but it looks good so far.
Al, did this fix end up in Linus's tree yet? I'd like to pull it into
the next 3.12-stable release, but will wait until Linus has it of
course.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60
2013-11-21 23:01 ` Greg KH
@ 2013-11-21 23:16 ` Michael Marineau
2013-11-22 19:45 ` Greg KH
2013-11-21 23:22 ` Al Viro
1 sibling, 1 reply; 9+ messages in thread
From: Michael Marineau @ 2013-11-21 23:16 UTC (permalink / raw)
To: Greg KH; +Cc: Al Viro, Waiman Long, linux-kernel
On Thu, Nov 21, 2013 at 3:01 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Nov 13, 2013 at 02:51:59PM -0800, Michael Marineau wrote:
>> On Wed, Nov 13, 2013 at 4:39 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> > On Wed, Nov 13, 2013 at 03:34:13AM -0800, Michael Marineau wrote:
>> >> Greetings,
>> >>
>> >> Commit 232d2d60aa5469bb097f55728f65146bd49c1d25 causes intermittent
>> >> errors in /proc/*/fd/* where readlink returns "/" instead of the
>> >> correct path. This can be reproduced by the script below which copies
>> >> the kernel source directory structure while obsessively looking up
>> >> directory fds in proc from another process. Reverting
>> >> 232d2d60aa5469bb097f55728f65146bd49c1d25 after two related commits
>> >> 48f5ec21d9c67e881ff35343988e290ef5cf933f
>> >> 1812997720ab90d029548778c55d7315555e1fef fixes the issue.
>> >
>> > Looking into it... It seems that we are getting to the end of
>> > prepend_path() with non-negative error and bptr == *buffer.
>> > What the...
>> >
>> > OK, I see what's going on. We never reinitialize dentry, vfsmount and mnt
>> > if we decide to restart. See if the following helps:
>> >
>> > diff --git a/fs/dcache.c b/fs/dcache.c
>> > index ae6ebb8..89f9671 100644
>> > --- a/fs/dcache.c
>> > +++ b/fs/dcache.c
>> > @@ -2881,9 +2881,9 @@ static int prepend_path(const struct path *path,
>> > const struct path *root,
>> > char **buffer, int *buflen)
>> > {
>> > - struct dentry *dentry = path->dentry;
>> > - struct vfsmount *vfsmnt = path->mnt;
>> > - struct mount *mnt = real_mount(vfsmnt);
>> > + struct dentry *dentry;
>> > + struct vfsmount *vfsmnt;
>> > + struct mount *mnt;
>> > int error = 0;
>> > unsigned seq = 0;
>> > char *bptr;
>> > @@ -2893,6 +2893,9 @@ static int prepend_path(const struct path *path,
>> > restart:
>> > bptr = *buffer;
>> > blen = *buflen;
>> > + dentry = path->dentry;
>> > + vfsmnt = path->mnt;
>> > + mnt = real_mount(vfsmnt);
>> > read_seqbegin_or_lock(&rename_lock, &seq);
>> > while (dentry != root->dentry || vfsmnt != root->mnt) {
>> > struct dentry * parent;
>>
>> That appears to do the trick! I've tried my test case against that
>> patch on both Linus' git tree (as of last night) and the 3.12 release.
>> I'm now running the long build job that initially stumbled across this
>> bug now but it looks good so far.
>
> Al, did this fix end up in Linus's tree yet? I'd like to pull it into
> the next 3.12-stable release, but will wait until Linus has it of
> course.
It did, I was going to poke you about it when I noticed no one got it
to you before 3.12.1 :)
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/fs/dcache.c?id=ede4cebce16f5643c61aedd6d88d9070a1d23a68
I don't know if there are any other fixes in this code that would also
be good for 3.12, there appears to be a number of fix-looking commits
to dcache.c
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60
2013-11-21 23:01 ` Greg KH
2013-11-21 23:16 ` Michael Marineau
@ 2013-11-21 23:22 ` Al Viro
2013-11-22 19:45 ` Greg KH
1 sibling, 1 reply; 9+ messages in thread
From: Al Viro @ 2013-11-21 23:22 UTC (permalink / raw)
To: Greg KH; +Cc: Michael Marineau, Waiman Long, linux-kernel
On Thu, Nov 21, 2013 at 03:01:07PM -0800, Greg KH wrote:
> Al, did this fix end up in Linus's tree yet? I'd like to pull it into
> the next 3.12-stable release, but will wait until Linus has it of
> course.
Yes - ede4ce.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60
2013-11-21 23:22 ` Al Viro
@ 2013-11-22 19:45 ` Greg KH
0 siblings, 0 replies; 9+ messages in thread
From: Greg KH @ 2013-11-22 19:45 UTC (permalink / raw)
To: Al Viro; +Cc: Michael Marineau, Waiman Long, linux-kernel
On Thu, Nov 21, 2013 at 11:22:42PM +0000, Al Viro wrote:
> On Thu, Nov 21, 2013 at 03:01:07PM -0800, Greg KH wrote:
>
> > Al, did this fix end up in Linus's tree yet? I'd like to pull it into
> > the next 3.12-stable release, but will wait until Linus has it of
> > course.
>
> Yes - ede4ce.
Thanks, now queued up.
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60
2013-11-21 23:16 ` Michael Marineau
@ 2013-11-22 19:45 ` Greg KH
0 siblings, 0 replies; 9+ messages in thread
From: Greg KH @ 2013-11-22 19:45 UTC (permalink / raw)
To: Michael Marineau; +Cc: Al Viro, Waiman Long, linux-kernel
On Thu, Nov 21, 2013 at 03:16:13PM -0800, Michael Marineau wrote:
> On Thu, Nov 21, 2013 at 3:01 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Nov 13, 2013 at 02:51:59PM -0800, Michael Marineau wrote:
> >> On Wed, Nov 13, 2013 at 4:39 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >> > On Wed, Nov 13, 2013 at 03:34:13AM -0800, Michael Marineau wrote:
> >> >> Greetings,
> >> >>
> >> >> Commit 232d2d60aa5469bb097f55728f65146bd49c1d25 causes intermittent
> >> >> errors in /proc/*/fd/* where readlink returns "/" instead of the
> >> >> correct path. This can be reproduced by the script below which copies
> >> >> the kernel source directory structure while obsessively looking up
> >> >> directory fds in proc from another process. Reverting
> >> >> 232d2d60aa5469bb097f55728f65146bd49c1d25 after two related commits
> >> >> 48f5ec21d9c67e881ff35343988e290ef5cf933f
> >> >> 1812997720ab90d029548778c55d7315555e1fef fixes the issue.
> >> >
> >> > Looking into it... It seems that we are getting to the end of
> >> > prepend_path() with non-negative error and bptr == *buffer.
> >> > What the...
> >> >
> >> > OK, I see what's going on. We never reinitialize dentry, vfsmount and mnt
> >> > if we decide to restart. See if the following helps:
> >> >
> >> > diff --git a/fs/dcache.c b/fs/dcache.c
> >> > index ae6ebb8..89f9671 100644
> >> > --- a/fs/dcache.c
> >> > +++ b/fs/dcache.c
> >> > @@ -2881,9 +2881,9 @@ static int prepend_path(const struct path *path,
> >> > const struct path *root,
> >> > char **buffer, int *buflen)
> >> > {
> >> > - struct dentry *dentry = path->dentry;
> >> > - struct vfsmount *vfsmnt = path->mnt;
> >> > - struct mount *mnt = real_mount(vfsmnt);
> >> > + struct dentry *dentry;
> >> > + struct vfsmount *vfsmnt;
> >> > + struct mount *mnt;
> >> > int error = 0;
> >> > unsigned seq = 0;
> >> > char *bptr;
> >> > @@ -2893,6 +2893,9 @@ static int prepend_path(const struct path *path,
> >> > restart:
> >> > bptr = *buffer;
> >> > blen = *buflen;
> >> > + dentry = path->dentry;
> >> > + vfsmnt = path->mnt;
> >> > + mnt = real_mount(vfsmnt);
> >> > read_seqbegin_or_lock(&rename_lock, &seq);
> >> > while (dentry != root->dentry || vfsmnt != root->mnt) {
> >> > struct dentry * parent;
> >>
> >> That appears to do the trick! I've tried my test case against that
> >> patch on both Linus' git tree (as of last night) and the 3.12 release.
> >> I'm now running the long build job that initially stumbled across this
> >> bug now but it looks good so far.
> >
> > Al, did this fix end up in Linus's tree yet? I'd like to pull it into
> > the next 3.12-stable release, but will wait until Linus has it of
> > course.
>
> It did, I was going to poke you about it when I noticed no one got it
> to you before 3.12.1 :)
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/fs/dcache.c?id=ede4cebce16f5643c61aedd6d88d9070a1d23a68
>
> I don't know if there are any other fixes in this code that would also
> be good for 3.12, there appears to be a number of fix-looking commits
> to dcache.c
There's a bunch of rcu changes in the file, but that's new stuff for
3.13, so they aren't relevant. If you see anything that you think is
needed, please let me know.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-11-22 19:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-13 11:34 3.12 Regression: dcache: Translating dentry into pathname without taking rename_lock 232d2d60 Michael Marineau
2013-11-13 12:39 ` Al Viro
2013-11-13 14:49 ` Long, Wai Man
2013-11-13 22:51 ` Michael Marineau
2013-11-21 23:01 ` Greg KH
2013-11-21 23:16 ` Michael Marineau
2013-11-22 19:45 ` Greg KH
2013-11-21 23:22 ` Al Viro
2013-11-22 19:45 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox