| Message ID | 20230420160646.291053-1-bharathsm.hsk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMB3: Add missing locks to protect deferred close file list | expand |
Looks good. Acked-by me. Can you add a "Fixes ..." line to the commit message. On Fri, 21 Apr 2023 at 02:16, Bharath SM <bharathsm.hsk@gmail.com> wrote: > > From: Bharath SM <bharathsm@microsoft.com> > > cifs_del_deferred_close function has a critical section which modifies > the deferred close file list. We must acquire deferred_lock before > calling cifs_del_deferred_close function. > > Signed-off-by: Bharath SM <bharathsm@microsoft.com> > --- > fs/cifs/misc.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c > index a0d286ee723d..89bbc12e2ca7 100644 > --- a/fs/cifs/misc.c > +++ b/fs/cifs/misc.c > @@ -742,7 +742,10 @@ cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode) > list_for_each_entry(cfile, &cifs_inode->openFileList, flist) { > if (delayed_work_pending(&cfile->deferred)) { > if (cancel_delayed_work(&cfile->deferred)) { > + > + spin_lock(&cifs_inode->deferred_lock); > cifs_del_deferred_close(cfile); > + spin_unlock(&cifs_inode->deferred_lock); > > tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); > if (tmp_list == NULL) > @@ -773,7 +776,10 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon) > list_for_each_entry(cfile, &tcon->openFileList, tlist) { > if (delayed_work_pending(&cfile->deferred)) { > if (cancel_delayed_work(&cfile->deferred)) { > + > + spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > cifs_del_deferred_close(cfile); > + spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > > tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); > if (tmp_list == NULL) > @@ -808,7 +814,10 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path) > if (strstr(full_path, path)) { > if (delayed_work_pending(&cfile->deferred)) { > if (cancel_delayed_work(&cfile->deferred)) { > + > + spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > cifs_del_deferred_close(cfile); > + spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > > tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); > if (tmp_list == NULL) > -- > 2.34.1 >
On Thu, Apr 20, 2023 at 9:54 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > Looks good. Acked-by me. > > Can you add a "Fixes ..." line to the commit message. cifs_del_deferred_close function has a critical section which modifies the deferred close file list. We must acquire deferred_lock before calling cifs_del_deferred_close function. Fixes: ca08d0eac020 ("cifs: Fix memory leak on the deferred close") Signed-off-by: Bharath SM <bharathsm@microsoft.com>
Bharath SM <bharathsm.hsk@gmail.com> writes: > From: Bharath SM <bharathsm@microsoft.com> > > cifs_del_deferred_close function has a critical section which modifies > the deferred close file list. We must acquire deferred_lock before > calling cifs_del_deferred_close function. > > Signed-off-by: Bharath SM <bharathsm@microsoft.com> > --- > fs/cifs/misc.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c > index a0d286ee723d..89bbc12e2ca7 100644 > --- a/fs/cifs/misc.c > +++ b/fs/cifs/misc.c > @@ -742,7 +742,10 @@ cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode) > list_for_each_entry(cfile, &cifs_inode->openFileList, flist) { > if (delayed_work_pending(&cfile->deferred)) { > if (cancel_delayed_work(&cfile->deferred)) { > + No need for this extra blank line. Please remove the below ones as well. With the "Fixes:" tag added as per Ronnie's suggestion, Acked-by: Paulo Alcantara (SUSE) <pc@manguebit.com> > + spin_lock(&cifs_inode->deferred_lock); > cifs_del_deferred_close(cfile); > + spin_unlock(&cifs_inode->deferred_lock); > > tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); > if (tmp_list == NULL) > @@ -773,7 +776,10 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon) > list_for_each_entry(cfile, &tcon->openFileList, tlist) { > if (delayed_work_pending(&cfile->deferred)) { > if (cancel_delayed_work(&cfile->deferred)) { > + > + spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > cifs_del_deferred_close(cfile); > + spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > > tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); > if (tmp_list == NULL) > @@ -808,7 +814,10 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path) > if (strstr(full_path, path)) { > if (delayed_work_pending(&cfile->deferred)) { > if (cancel_delayed_work(&cfile->deferred)) { > + > + spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > cifs_del_deferred_close(cfile); > + spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > > tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); > if (tmp_list == NULL) > -- > 2.34.1
On Thu, Apr 20, 2023 at 11:15 PM Paulo Alcantara <pc@manguebit.com> wrote: > > Bharath SM <bharathsm.hsk@gmail.com> writes: > > > From: Bharath SM <bharathsm@microsoft.com> > > > > cifs_del_deferred_close function has a critical section which modifies > > the deferred close file list. We must acquire deferred_lock before > > calling cifs_del_deferred_close function. > > > > Signed-off-by: Bharath SM <bharathsm@microsoft.com> > > --- > > fs/cifs/misc.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c > > index a0d286ee723d..89bbc12e2ca7 100644 > > --- a/fs/cifs/misc.c > > +++ b/fs/cifs/misc.c > > @@ -742,7 +742,10 @@ cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode) > > list_for_each_entry(cfile, &cifs_inode->openFileList, flist) { > > if (delayed_work_pending(&cfile->deferred)) { > > if (cancel_delayed_work(&cfile->deferred)) { > > + > > No need for this extra blank line. Please remove the below ones as > well. > > With the "Fixes:" tag added as per Ronnie's suggestion, > > Acked-by: Paulo Alcantara (SUSE) <pc@manguebit.com> Done. Please find updated patch in attachments.
added to cifs-2.6.git for-next now (with the 2 acked-bys) that Fixes tag is there On Thu, Apr 20, 2023 at 1:08 PM Paulo Alcantara <pc@manguebit.com> wrote: > > Bharath SM <bharathsm.hsk@gmail.com> writes: > > > From: Bharath SM <bharathsm@microsoft.com> > > > > cifs_del_deferred_close function has a critical section which modifies > > the deferred close file list. We must acquire deferred_lock before > > calling cifs_del_deferred_close function. > > > > Signed-off-by: Bharath SM <bharathsm@microsoft.com> > > --- > > fs/cifs/misc.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c > > index a0d286ee723d..89bbc12e2ca7 100644 > > --- a/fs/cifs/misc.c > > +++ b/fs/cifs/misc.c > > @@ -742,7 +742,10 @@ cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode) > > list_for_each_entry(cfile, &cifs_inode->openFileList, flist) { > > if (delayed_work_pending(&cfile->deferred)) { > > if (cancel_delayed_work(&cfile->deferred)) { > > + > > No need for this extra blank line. Please remove the below ones as > well. > > With the "Fixes:" tag added as per Ronnie's suggestion, > > Acked-by: Paulo Alcantara (SUSE) <pc@manguebit.com> > > > + spin_lock(&cifs_inode->deferred_lock); > > cifs_del_deferred_close(cfile); > > + spin_unlock(&cifs_inode->deferred_lock); > > > > tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); > > if (tmp_list == NULL) > > @@ -773,7 +776,10 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon) > > list_for_each_entry(cfile, &tcon->openFileList, tlist) { > > if (delayed_work_pending(&cfile->deferred)) { > > if (cancel_delayed_work(&cfile->deferred)) { > > + > > + spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > > cifs_del_deferred_close(cfile); > > + spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > > > > tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); > > if (tmp_list == NULL) > > @@ -808,7 +814,10 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path) > > if (strstr(full_path, path)) { > > if (delayed_work_pending(&cfile->deferred)) { > > if (cancel_delayed_work(&cfile->deferred)) { > > + > > + spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > > cifs_del_deferred_close(cfile); > > + spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); > > > > tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); > > if (tmp_list == NULL) > > -- > > 2.34.1
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c index a0d286ee723d..89bbc12e2ca7 100644 --- a/fs/cifs/misc.c +++ b/fs/cifs/misc.c @@ -742,7 +742,10 @@ cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode) list_for_each_entry(cfile, &cifs_inode->openFileList, flist) { if (delayed_work_pending(&cfile->deferred)) { if (cancel_delayed_work(&cfile->deferred)) { + + spin_lock(&cifs_inode->deferred_lock); cifs_del_deferred_close(cfile); + spin_unlock(&cifs_inode->deferred_lock); tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); if (tmp_list == NULL) @@ -773,7 +776,10 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon) list_for_each_entry(cfile, &tcon->openFileList, tlist) { if (delayed_work_pending(&cfile->deferred)) { if (cancel_delayed_work(&cfile->deferred)) { + + spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); cifs_del_deferred_close(cfile); + spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); if (tmp_list == NULL) @@ -808,7 +814,10 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path) if (strstr(full_path, path)) { if (delayed_work_pending(&cfile->deferred)) { if (cancel_delayed_work(&cfile->deferred)) { + + spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); cifs_del_deferred_close(cfile); + spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock); tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC); if (tmp_list == NULL)