| Message ID | 20231229111618.38887-2-sprasad@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] cifs: cifs_chan_is_iface_active should be called with chan_lock held | expand |
On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote: > > From: Shyam Prasad N <sprasad@microsoft.com> > > parse_server_interfaces should be in complete charge of maintaining > the iface_list linked list. Today, iface entries are removed > from the list only when the last refcount is dropped. > i.e. in release_iface. However, this can result in undercounting > of refcount if the server stops advertising interfaces (which > Azure SMB server does). > > This change puts parse_server_interfaces in full charge of > maintaining the iface_list. So if an empty list is returned > by the server, the entries in the list will immediately be > removed. This way, a following call to the same function will > not find entries in the list. > > Fixes: aa45dadd34e4 ("cifs: change iface_list from array to sorted linked list") > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > --- > fs/smb/client/cifsglob.h | 1 - > fs/smb/client/smb2ops.c | 27 +++++++++++++++++---------- > 2 files changed, 17 insertions(+), 11 deletions(-) > > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h > index ba80c854c9ca..f840756e0169 100644 > --- a/fs/smb/client/cifsglob.h > +++ b/fs/smb/client/cifsglob.h > @@ -1014,7 +1014,6 @@ release_iface(struct kref *ref) > struct cifs_server_iface *iface = container_of(ref, > struct cifs_server_iface, > refcount); > - list_del_init(&iface->iface_head); > kfree(iface); > } > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > index 104c58df0368..b813485c0e86 100644 > --- a/fs/smb/client/smb2ops.c > +++ b/fs/smb/client/smb2ops.c > @@ -595,16 +595,12 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf, > } > > /* > - * Go through iface_list and do kref_put to remove > - * any unused ifaces. ifaces in use will be removed > - * when the last user calls a kref_put on it > + * Go through iface_list and mark them as inactive > */ > list_for_each_entry_safe(iface, niface, &ses->iface_list, > - iface_head) { > + iface_head) > iface->is_active = 0; > - kref_put(&iface->refcount, release_iface); > - ses->iface_count--; > - } > + > spin_unlock(&ses->iface_lock); > > /* > @@ -678,10 +674,7 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf, > iface_head) { > ret = iface_cmp(iface, &tmp_iface); > if (!ret) { > - /* just get a ref so that it doesn't get picked/freed */ > iface->is_active = 1; > - kref_get(&iface->refcount); > - ses->iface_count++; > spin_unlock(&ses->iface_lock); > goto next_iface; > } else if (ret < 0) { > @@ -748,6 +741,20 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf, > } > > out: > + /* > + * Go through the list again and put the inactive entries > + */ > + spin_lock(&ses->iface_lock); > + list_for_each_entry_safe(iface, niface, &ses->iface_list, > + iface_head) { > + if (!iface->is_active) { > + list_del(&iface->iface_head); > + kref_put(&iface->refcount, release_iface); > + ses->iface_count--; > + } > + } > + spin_unlock(&ses->iface_lock); > + > return rc; > } > > -- > 2.34.1 > Hi Steve.. This one should also be marked for stable. I missed tagging it before I sent.
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h index ba80c854c9ca..f840756e0169 100644 --- a/fs/smb/client/cifsglob.h +++ b/fs/smb/client/cifsglob.h @@ -1014,7 +1014,6 @@ release_iface(struct kref *ref) struct cifs_server_iface *iface = container_of(ref, struct cifs_server_iface, refcount); - list_del_init(&iface->iface_head); kfree(iface); } diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index 104c58df0368..b813485c0e86 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -595,16 +595,12 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf, } /* - * Go through iface_list and do kref_put to remove - * any unused ifaces. ifaces in use will be removed - * when the last user calls a kref_put on it + * Go through iface_list and mark them as inactive */ list_for_each_entry_safe(iface, niface, &ses->iface_list, - iface_head) { + iface_head) iface->is_active = 0; - kref_put(&iface->refcount, release_iface); - ses->iface_count--; - } + spin_unlock(&ses->iface_lock); /* @@ -678,10 +674,7 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf, iface_head) { ret = iface_cmp(iface, &tmp_iface); if (!ret) { - /* just get a ref so that it doesn't get picked/freed */ iface->is_active = 1; - kref_get(&iface->refcount); - ses->iface_count++; spin_unlock(&ses->iface_lock); goto next_iface; } else if (ret < 0) { @@ -748,6 +741,20 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf, } out: + /* + * Go through the list again and put the inactive entries + */ + spin_lock(&ses->iface_lock); + list_for_each_entry_safe(iface, niface, &ses->iface_list, + iface_head) { + if (!iface->is_active) { + list_del(&iface->iface_head); + kref_put(&iface->refcount, release_iface); + ses->iface_count--; + } + } + spin_unlock(&ses->iface_lock); + return rc; }