diff mbox series

SMB3: Add missing locks to protect deferred close file list

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

Commit Message

Bharath SM April 20, 2023, 4:06 p.m. UTC
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(+)

Comments

ronnie sahlberg April 20, 2023, 4:24 p.m. UTC | #1
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
>
Bharath SM April 20, 2023, 5:06 p.m. UTC | #2
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>
Paulo Alcantara April 20, 2023, 5:45 p.m. UTC | #3
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
Bharath SM April 20, 2023, 7:14 p.m. UTC | #4
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.
Steve French April 27, 2023, 4:05 p.m. UTC | #5
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 mbox series

Patch

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)