在 2021-01-21星期四的 09:07 +0100,Miklos Szeredi写道: > On Thu, Jan 21, 2021 at 4:43 AM Icenowy Zheng <icen...@aosc.io> > 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 <icen...@aosc.io> > > > > 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 <icen...@aosc.io> > > > > > --- > > > > > 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 <icen...@aosc.io> > > 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