Re: [PATCH v3] ovl: use a dedicated semaphore for dir upperfile caching
在 2021-01-21星期四的 09:07 +0100,Miklos Szeredi写道: > On Thu, Jan 21, 2021 at 4:43 AM Icenowy Zheng > wrote: > > > > 在 2021-01-20星期三的 11:20 +0100,Miklos Szeredi写道: > > > On Tue, Jan 05, 2021 at 08:47:41AM +0200, Amir Goldstein wrote: > > > > On Tue, Jan 5, 2021 at 2:36 AM Icenowy Zheng > > > > wrote: > > > > > > > > > > The function ovl_dir_real_file() currently uses the semaphore > > > > > of > > > > > the > > > > > inode to synchronize write to the upperfile cache field. > > > > > > > > Although the inode lock is a rw_sem it is referred to as the > > > > "inode > > > > lock" > > > > and you also left semaphore in the commit subject. > > > > No need to re-post. This can be fixed on commit. > > > > > > > > > > > > > > However, this function will get called by > > > > > ovl_ioctl_set_flags(), > > > > > which > > > > > utilizes the inode semaphore too. In this case > > > > > ovl_dir_real_file() will > > > > > try to claim a lock that is owned by a function in its call > > > > > stack, which > > > > > won't get released before ovl_dir_real_file() returns. > > > > > > > > > > Define a dedicated semaphore for the upperfile cache, so that > > > > > the > > > > > deadlock won't happen. > > > > > > > > > > Fixes: 61536bed2149 ("ovl: support [S|G]ETFLAGS and > > > > > FS[S|G]ETXATTR ioctls for directories") > > > > > Cc: sta...@vger.kernel.org # v5.10 > > > > > Signed-off-by: Icenowy Zheng > > > > > --- > > > > > Changes in v2: > > > > > - Fixed missing replacement in error handling path. > > > > > Changes in v3: > > > > > - Use mutex instead of semaphore. > > > > > > > > > > fs/overlayfs/readdir.c | 10 +- > > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c > > > > > index 01620ebae1bd..3980f9982f34 100644 > > > > > --- a/fs/overlayfs/readdir.c > > > > > +++ b/fs/overlayfs/readdir.c > > > > > @@ -56,6 +56,7 @@ struct ovl_dir_file { > > > > > struct list_head *cursor; > > > > > struct file *realfile; > > > > > struct file *upperfile; > > > > > + struct mutex upperfile_mutex; > > > > > > > > That's a very specific name. > > > > This mutex protects members of struct ovl_dir_file, which could > > > > evolve > > > > into struct ovl_file one day (because no reason to cache only > > > > dir > > > > upper file), > > > > so I would go with a more generic name, but let's leave it to > > > > Miklos to decide. > > > > > > > > He could have a different idea altogether for fixing this bug. > > > > > > How about this (untested) patch? > > > > > > It's a cleanup as well as a fix, but maybe we should separate the > > > cleanup from > > > the fix... > > > > If you are going to post this, feel free to add > > > > Tested-by: Icenowy Zheng > > Okay, thanks. > > > (And if you remove the IS_ERR(realfile) part, the tested-by tag > > still > > applies.) > > Dropping the IS_ERR(realfile) here would mean having to add the same > check before relevant fput() calls, which would make it more complex > not less. > > Or did you mean something else? I mean "seperate the cleanup from the fix". This is only for when you do the seperation. > > Thanks, > Miklos
Re: [PATCH v3] ovl: use a dedicated semaphore for dir upperfile caching
On Thu, Jan 21, 2021 at 4:43 AM Icenowy Zheng wrote: > > 在 2021-01-20星期三的 11:20 +0100,Miklos Szeredi写道: > > On Tue, Jan 05, 2021 at 08:47:41AM +0200, Amir Goldstein wrote: > > > On Tue, Jan 5, 2021 at 2:36 AM Icenowy Zheng > > > wrote: > > > > > > > > The function ovl_dir_real_file() currently uses the semaphore of > > > > the > > > > inode to synchronize write to the upperfile cache field. > > > > > > Although the inode lock is a rw_sem it is referred to as the "inode > > > lock" > > > and you also left semaphore in the commit subject. > > > No need to re-post. This can be fixed on commit. > > > > > > > > > > > However, this function will get called by ovl_ioctl_set_flags(), > > > > which > > > > utilizes the inode semaphore too. In this case > > > > ovl_dir_real_file() will > > > > try to claim a lock that is owned by a function in its call > > > > stack, which > > > > won't get released before ovl_dir_real_file() returns. > > > > > > > > Define a dedicated semaphore for the upperfile cache, so that the > > > > deadlock won't happen. > > > > > > > > Fixes: 61536bed2149 ("ovl: support [S|G]ETFLAGS and > > > > FS[S|G]ETXATTR ioctls for directories") > > > > Cc: sta...@vger.kernel.org # v5.10 > > > > Signed-off-by: Icenowy Zheng > > > > --- > > > > Changes in v2: > > > > - Fixed missing replacement in error handling path. > > > > Changes in v3: > > > > - Use mutex instead of semaphore. > > > > > > > > fs/overlayfs/readdir.c | 10 +- > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c > > > > index 01620ebae1bd..3980f9982f34 100644 > > > > --- a/fs/overlayfs/readdir.c > > > > +++ b/fs/overlayfs/readdir.c > > > > @@ -56,6 +56,7 @@ struct ovl_dir_file { > > > > struct list_head *cursor; > > > > struct file *realfile; > > > > struct file *upperfile; > > > > + struct mutex upperfile_mutex; > > > > > > That's a very specific name. > > > This mutex protects members of struct ovl_dir_file, which could > > > evolve > > > into struct ovl_file one day (because no reason to cache only dir > > > upper file), > > > so I would go with a more generic name, but let's leave it to > > > Miklos to decide. > > > > > > He could have a different idea altogether for fixing this bug. > > > > How about this (untested) patch? > > > > It's a cleanup as well as a fix, but maybe we should separate the > > cleanup from > > the fix... > > If you are going to post this, feel free to add > > Tested-by: Icenowy Zheng Okay, thanks. > (And if you remove the IS_ERR(realfile) part, the tested-by tag still > applies.) Dropping the IS_ERR(realfile) here would mean having to add the same check before relevant fput() calls, which would make it more complex not less. Or did you mean something else? Thanks, Miklos
Re: [PATCH v3] ovl: use a dedicated semaphore for dir upperfile caching
在 2021-01-20星期三的 11:20 +0100,Miklos Szeredi写道: > On Tue, Jan 05, 2021 at 08:47:41AM +0200, Amir Goldstein wrote: > > On Tue, Jan 5, 2021 at 2:36 AM Icenowy Zheng > > wrote: > > > > > > The function ovl_dir_real_file() currently uses the semaphore of > > > the > > > inode to synchronize write to the upperfile cache field. > > > > Although the inode lock is a rw_sem it is referred to as the "inode > > lock" > > and you also left semaphore in the commit subject. > > No need to re-post. This can be fixed on commit. > > > > > > > > However, this function will get called by ovl_ioctl_set_flags(), > > > which > > > utilizes the inode semaphore too. In this case > > > ovl_dir_real_file() will > > > try to claim a lock that is owned by a function in its call > > > stack, which > > > won't get released before ovl_dir_real_file() returns. > > > > > > Define a dedicated semaphore for the upperfile cache, so that the > > > deadlock won't happen. > > > > > > Fixes: 61536bed2149 ("ovl: support [S|G]ETFLAGS and > > > FS[S|G]ETXATTR ioctls for directories") > > > Cc: sta...@vger.kernel.org # v5.10 > > > Signed-off-by: Icenowy Zheng > > > --- > > > Changes in v2: > > > - Fixed missing replacement in error handling path. > > > Changes in v3: > > > - Use mutex instead of semaphore. > > > > > > fs/overlayfs/readdir.c | 10 +- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c > > > index 01620ebae1bd..3980f9982f34 100644 > > > --- a/fs/overlayfs/readdir.c > > > +++ b/fs/overlayfs/readdir.c > > > @@ -56,6 +56,7 @@ struct ovl_dir_file { > > > struct list_head *cursor; > > > struct file *realfile; > > > struct file *upperfile; > > > + struct mutex upperfile_mutex; > > > > That's a very specific name. > > This mutex protects members of struct ovl_dir_file, which could > > evolve > > into struct ovl_file one day (because no reason to cache only dir > > upper file), > > so I would go with a more generic name, but let's leave it to > > Miklos to decide. > > > > He could have a different idea altogether for fixing this bug. > > How about this (untested) patch? > > It's a cleanup as well as a fix, but maybe we should separate the > cleanup from > the fix... If you are going to post this, feel free to add Tested-by: Icenowy Zheng (And if you remove the IS_ERR(realfile) part, the tested-by tag still applies.) > > Thanks, > Miklos > --- > > fs/overlayfs/readdir.c | 23 +++ > 1 file changed, 7 insertions(+), 16 deletions(-) > > --- a/fs/overlayfs/readdir.c > +++ b/fs/overlayfs/readdir.c > @@ -865,7 +865,7 @@ struct file *ovl_dir_real_file(const str > > struct ovl_dir_file *od = file->private_data; > struct dentry *dentry = file->f_path.dentry; > - struct file *realfile = od->realfile; > + struct file *old, *realfile = od->realfile; > > if (!OVL_TYPE_UPPER(ovl_path_type(dentry))) > return want_upper ? NULL : realfile; > @@ -874,29 +874,20 @@ struct file *ovl_dir_real_file(const str > * Need to check if we started out being a lower dir, but got > copied up > */ > if (!od->is_upper) { > - struct inode *inode = file_inode(file); > - > realfile = READ_ONCE(od->upperfile); > if (!realfile) { > struct path upperpath; > > ovl_path_upper(dentry, ); > realfile = ovl_dir_open_realfile(file, > ); > + if (IS_ERR(realfile)) > + return realfile; > > - inode_lock(inode); > - if (!od->upperfile) { > - if (IS_ERR(realfile)) { > - inode_unlock(inode); > - return realfile; > - } > - smp_store_release(>upperfile, > realfile); > - } else { > - /* somebody has beaten us to it */ > - if (!IS_ERR(realfile)) > - fput(realfile); > - realfile = od->upperfile; > + old = cmpxchg_release(>upperfile, NULL, > realfile); > + if (old) { > + fput(realfile); > + realfile = old; > } > - inode_unlock(inode); > } > } >
Re: [PATCH v3] ovl: use a dedicated semaphore for dir upperfile caching
On Wed, Jan 20, 2021 at 12:20 PM Miklos Szeredi wrote: > > On Tue, Jan 05, 2021 at 08:47:41AM +0200, Amir Goldstein wrote: > > On Tue, Jan 5, 2021 at 2:36 AM Icenowy Zheng wrote: > > > > > > The function ovl_dir_real_file() currently uses the semaphore of the > > > inode to synchronize write to the upperfile cache field. > > > > Although the inode lock is a rw_sem it is referred to as the "inode lock" > > and you also left semaphore in the commit subject. > > No need to re-post. This can be fixed on commit. > > > > > > > > However, this function will get called by ovl_ioctl_set_flags(), which > > > utilizes the inode semaphore too. In this case ovl_dir_real_file() will > > > try to claim a lock that is owned by a function in its call stack, which > > > won't get released before ovl_dir_real_file() returns. > > > > > > Define a dedicated semaphore for the upperfile cache, so that the > > > deadlock won't happen. > > > > > > Fixes: 61536bed2149 ("ovl: support [S|G]ETFLAGS and FS[S|G]ETXATTR ioctls > > > for directories") > > > Cc: sta...@vger.kernel.org # v5.10 > > > Signed-off-by: Icenowy Zheng > > > --- > > > Changes in v2: > > > - Fixed missing replacement in error handling path. > > > Changes in v3: > > > - Use mutex instead of semaphore. > > > > > > fs/overlayfs/readdir.c | 10 +- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c > > > index 01620ebae1bd..3980f9982f34 100644 > > > --- a/fs/overlayfs/readdir.c > > > +++ b/fs/overlayfs/readdir.c > > > @@ -56,6 +56,7 @@ struct ovl_dir_file { > > > struct list_head *cursor; > > > struct file *realfile; > > > struct file *upperfile; > > > + struct mutex upperfile_mutex; > > > > That's a very specific name. > > This mutex protects members of struct ovl_dir_file, which could evolve > > into struct ovl_file one day (because no reason to cache only dir upper > > file), > > so I would go with a more generic name, but let's leave it to Miklos to > > decide. > > > > He could have a different idea altogether for fixing this bug. > > How about this (untested) patch? > Much better :) > It's a cleanup as well as a fix, but maybe we should separate the cleanup from > the fix... > > Thanks, > Miklos > --- > > fs/overlayfs/readdir.c | 23 +++ > 1 file changed, 7 insertions(+), 16 deletions(-) > > --- a/fs/overlayfs/readdir.c > +++ b/fs/overlayfs/readdir.c > @@ -865,7 +865,7 @@ struct file *ovl_dir_real_file(const str > > struct ovl_dir_file *od = file->private_data; > struct dentry *dentry = file->f_path.dentry; > - struct file *realfile = od->realfile; > + struct file *old, *realfile = od->realfile; > > if (!OVL_TYPE_UPPER(ovl_path_type(dentry))) > return want_upper ? NULL : realfile; > @@ -874,29 +874,20 @@ struct file *ovl_dir_real_file(const str > * Need to check if we started out being a lower dir, but got copied > up > */ > if (!od->is_upper) { > - struct inode *inode = file_inode(file); > - > realfile = READ_ONCE(od->upperfile); > if (!realfile) { > struct path upperpath; > > ovl_path_upper(dentry, ); > realfile = ovl_dir_open_realfile(file, ); > + if (IS_ERR(realfile)) > + return realfile; > > - inode_lock(inode); > - if (!od->upperfile) { > - if (IS_ERR(realfile)) { > - inode_unlock(inode); > - return realfile; > - } > - smp_store_release(>upperfile, realfile); > - } else { > - /* somebody has beaten us to it */ > - if (!IS_ERR(realfile)) > - fput(realfile); > - realfile = od->upperfile; > + old = cmpxchg_release(>upperfile, NULL, realfile); > + if (old) { > + fput(realfile); > + realfile = old; > } > - inode_unlock(inode); > } > } >
Re: [PATCH v3] ovl: use a dedicated semaphore for dir upperfile caching
On Tue, Jan 05, 2021 at 08:47:41AM +0200, Amir Goldstein wrote: > On Tue, Jan 5, 2021 at 2:36 AM Icenowy Zheng wrote: > > > > The function ovl_dir_real_file() currently uses the semaphore of the > > inode to synchronize write to the upperfile cache field. > > Although the inode lock is a rw_sem it is referred to as the "inode lock" > and you also left semaphore in the commit subject. > No need to re-post. This can be fixed on commit. > > > > > However, this function will get called by ovl_ioctl_set_flags(), which > > utilizes the inode semaphore too. In this case ovl_dir_real_file() will > > try to claim a lock that is owned by a function in its call stack, which > > won't get released before ovl_dir_real_file() returns. > > > > Define a dedicated semaphore for the upperfile cache, so that the > > deadlock won't happen. > > > > Fixes: 61536bed2149 ("ovl: support [S|G]ETFLAGS and FS[S|G]ETXATTR ioctls > > for directories") > > Cc: sta...@vger.kernel.org # v5.10 > > Signed-off-by: Icenowy Zheng > > --- > > Changes in v2: > > - Fixed missing replacement in error handling path. > > Changes in v3: > > - Use mutex instead of semaphore. > > > > fs/overlayfs/readdir.c | 10 +- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c > > index 01620ebae1bd..3980f9982f34 100644 > > --- a/fs/overlayfs/readdir.c > > +++ b/fs/overlayfs/readdir.c > > @@ -56,6 +56,7 @@ struct ovl_dir_file { > > struct list_head *cursor; > > struct file *realfile; > > struct file *upperfile; > > + struct mutex upperfile_mutex; > > That's a very specific name. > This mutex protects members of struct ovl_dir_file, which could evolve > into struct ovl_file one day (because no reason to cache only dir upper file), > so I would go with a more generic name, but let's leave it to Miklos to > decide. > > He could have a different idea altogether for fixing this bug. How about this (untested) patch? It's a cleanup as well as a fix, but maybe we should separate the cleanup from the fix... Thanks, Miklos --- fs/overlayfs/readdir.c | 23 +++ 1 file changed, 7 insertions(+), 16 deletions(-) --- a/fs/overlayfs/readdir.c +++ b/fs/overlayfs/readdir.c @@ -865,7 +865,7 @@ struct file *ovl_dir_real_file(const str struct ovl_dir_file *od = file->private_data; struct dentry *dentry = file->f_path.dentry; - struct file *realfile = od->realfile; + struct file *old, *realfile = od->realfile; if (!OVL_TYPE_UPPER(ovl_path_type(dentry))) return want_upper ? NULL : realfile; @@ -874,29 +874,20 @@ struct file *ovl_dir_real_file(const str * Need to check if we started out being a lower dir, but got copied up */ if (!od->is_upper) { - struct inode *inode = file_inode(file); - realfile = READ_ONCE(od->upperfile); if (!realfile) { struct path upperpath; ovl_path_upper(dentry, ); realfile = ovl_dir_open_realfile(file, ); + if (IS_ERR(realfile)) + return realfile; - inode_lock(inode); - if (!od->upperfile) { - if (IS_ERR(realfile)) { - inode_unlock(inode); - return realfile; - } - smp_store_release(>upperfile, realfile); - } else { - /* somebody has beaten us to it */ - if (!IS_ERR(realfile)) - fput(realfile); - realfile = od->upperfile; + old = cmpxchg_release(>upperfile, NULL, realfile); + if (old) { + fput(realfile); + realfile = old; } - inode_unlock(inode); } }
Re: [PATCH v3] ovl: use a dedicated semaphore for dir upperfile caching
On Tue, Jan 5, 2021 at 8:47 AM Amir Goldstein wrote: > > On Tue, Jan 5, 2021 at 2:36 AM Icenowy Zheng wrote: > > > > The function ovl_dir_real_file() currently uses the semaphore of the > > inode to synchronize write to the upperfile cache field. > > Although the inode lock is a rw_sem it is referred to as the "inode lock" > and you also left semaphore in the commit subject. > No need to re-post. This can be fixed on commit. > > > > > However, this function will get called by ovl_ioctl_set_flags(), which > > utilizes the inode semaphore too. In this case ovl_dir_real_file() will > > try to claim a lock that is owned by a function in its call stack, which > > won't get released before ovl_dir_real_file() returns. > > > > Define a dedicated semaphore for the upperfile cache, so that the > > deadlock won't happen. > > > > Fixes: 61536bed2149 ("ovl: support [S|G]ETFLAGS and FS[S|G]ETXATTR ioctls > > for directories") > > Cc: sta...@vger.kernel.org # v5.10 > > Signed-off-by: Icenowy Zheng > > --- > > Changes in v2: > > - Fixed missing replacement in error handling path. > > Changes in v3: > > - Use mutex instead of semaphore. > > > > fs/overlayfs/readdir.c | 10 +- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c > > index 01620ebae1bd..3980f9982f34 100644 > > --- a/fs/overlayfs/readdir.c > > +++ b/fs/overlayfs/readdir.c > > @@ -56,6 +56,7 @@ struct ovl_dir_file { > > struct list_head *cursor; > > struct file *realfile; > > struct file *upperfile; > > + struct mutex upperfile_mutex; > > That's a very specific name. > This mutex protects members of struct ovl_dir_file, which could evolve > into struct ovl_file one day (because no reason to cache only dir upper file), > so I would go with a more generic name, but let's leave it to Miklos to > decide. > > He could have a different idea altogether for fixing this bug. > Miklos, Please fast track this or an alternative fix. It fixes an easy to reproduce deadlock introduced in 5.10. Icenowy Zheng has written a simple xfstest reproducer, but it wasn't posted - best to avoid hanging tester's machines until a fix is merged... Thanks, Amir.
Re: [PATCH v3] ovl: use a dedicated semaphore for dir upperfile caching
On Tue, Jan 5, 2021 at 2:36 AM Icenowy Zheng wrote: > > The function ovl_dir_real_file() currently uses the semaphore of the > inode to synchronize write to the upperfile cache field. Although the inode lock is a rw_sem it is referred to as the "inode lock" and you also left semaphore in the commit subject. No need to re-post. This can be fixed on commit. > > However, this function will get called by ovl_ioctl_set_flags(), which > utilizes the inode semaphore too. In this case ovl_dir_real_file() will > try to claim a lock that is owned by a function in its call stack, which > won't get released before ovl_dir_real_file() returns. > > Define a dedicated semaphore for the upperfile cache, so that the > deadlock won't happen. > > Fixes: 61536bed2149 ("ovl: support [S|G]ETFLAGS and FS[S|G]ETXATTR ioctls for > directories") > Cc: sta...@vger.kernel.org # v5.10 > Signed-off-by: Icenowy Zheng > --- > Changes in v2: > - Fixed missing replacement in error handling path. > Changes in v3: > - Use mutex instead of semaphore. > > fs/overlayfs/readdir.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c > index 01620ebae1bd..3980f9982f34 100644 > --- a/fs/overlayfs/readdir.c > +++ b/fs/overlayfs/readdir.c > @@ -56,6 +56,7 @@ struct ovl_dir_file { > struct list_head *cursor; > struct file *realfile; > struct file *upperfile; > + struct mutex upperfile_mutex; That's a very specific name. This mutex protects members of struct ovl_dir_file, which could evolve into struct ovl_file one day (because no reason to cache only dir upper file), so I would go with a more generic name, but let's leave it to Miklos to decide. He could have a different idea altogether for fixing this bug. Thanks, Amir.