[PATCH next] smb: client: Fix NULL vs ERR_PTR() returns in cifs_get_tcon_super()

[PATCH next] smb: client: Fix NULL vs ERR_PTR() returns in cifs_get_tcon_super()

[Dan Carpenter]

The cifs_get_tcon_super() function returns NULL on error but the caller
expect it to return error pointers instead.  Change it to return error
pointers.  Otherwise it results in a NULL pointer dereference.

Fixes: 0938b093b1ae ("smb: client: Fix mount deadlock by avoiding super block iteration in DFS reconnect")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 fs/smb/client/misc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 3b6920a52daa..d73c36862e97 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -1116,7 +1116,7 @@ static struct super_block *cifs_get_tcon_super(struct cifs_tcon *tcon)
 	struct cifs_sb_info *cifs_sb;
 
 	if (!tcon)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 
 	spin_lock(&tcon->sb_list_lock);
 	list_for_each_entry(cifs_sb, &tcon->cifs_sb_list, tcon_sb_link) {
@@ -1141,7 +1141,7 @@ static struct super_block *cifs_get_tcon_super(struct cifs_tcon *tcon)
 	}
 	spin_unlock(&tcon->sb_list_lock);
 
-	return NULL;
+	return ERR_PTR(-ENOENT);
 }
 
 struct super_block *cifs_get_dfs_tcon_super(struct cifs_tcon *tcon)
-- 
2.47.2

Re: [PATCH next] smb: client: Fix NULL vs ERR_PTR() returns in cifs_get_tcon_super()

[Wang Zhaolong]

> The cifs_get_tcon_super() function returns NULL on error but the caller
> expect it to return error pointers instead.  Change it to return error
> pointers.  Otherwise it results in a NULL pointer dereference.
> 
> Fixes: 0938b093b1ae ("smb: client: Fix mount deadlock by avoiding super block iteration in DFS reconnect")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Hi Dan,

Thank you for your patch and for taking the time to address this issue.

I would like to mention that I have recently sent out the V4 version of
the patch series, which addresses the issues related to `cifs_get_tcon_super()`.
In the latest version, the issue of NULL pointer dereference has already
been resolved.

https://lore.kernel.org/all/CAH2r5msLMNdqdo6EBuTvrQ0hwrqSRC-LSZuN2WpwV+PkDwsCOw@mail.gmail.com/

I avoid null pointer dereferencing by performing a null pointer check on
the return value of cifs_get_dfs_tcon_super().


> ---
>   fs/smb/client/misc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> index 3b6920a52daa..d73c36862e97 100644
> --- a/fs/smb/client/misc.c
> +++ b/fs/smb/client/misc.c
> @@ -1116,7 +1116,7 @@ static struct super_block *cifs_get_tcon_super(struct cifs_tcon *tcon)
>   	struct cifs_sb_info *cifs_sb;
>   
>   	if (!tcon)
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>   
>   	spin_lock(&tcon->sb_list_lock);
>   	list_for_each_entry(cifs_sb, &tcon->cifs_sb_list, tcon_sb_link) {
> @@ -1141,7 +1141,7 @@ static struct super_block *cifs_get_tcon_super(struct cifs_tcon *tcon)
>   	}
>   	spin_unlock(&tcon->sb_list_lock);
>   
> -	return NULL;
> +	return ERR_PTR(-ENOENT);
>   }
>   
>   struct super_block *cifs_get_dfs_tcon_super(struct cifs_tcon *tcon)

Additionally, I think it somewhat peculiar that in the current
implementation, cifs_get_tcon_super() returns -EINVAL.

I would greatly appreciate it if you could review my latest patch series to
confirm if it resolves the concerns. If there are any additional improvements, I
would be happy to collaborate further to ensure the best possible solution.

Best regards,
Wang Zhaolong

Re: [PATCH next] smb: client: Fix NULL vs ERR_PTR() returns in cifs_get_tcon_super()

[Steve French]

Since Paulo pointed out a problem with v4 of this patch, an obvious
question is Dan's patch "independent enough" to take or would it make
the v5 of your patch harder.  Let me know when there is a v5 of the
patch so we can do more testing and review

On Mon, Aug 18, 2025 at 8:30 AM Wang Zhaolong
<wangzhaolong@huaweicloud.com> wrote:
>
>
>
> > The cifs_get_tcon_super() function returns NULL on error but the caller
> > expect it to return error pointers instead.  Change it to return error
> > pointers.  Otherwise it results in a NULL pointer dereference.
> >
> > Fixes: 0938b093b1ae ("smb: client: Fix mount deadlock by avoiding super block iteration in DFS reconnect")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>
> Hi Dan,
>
> Thank you for your patch and for taking the time to address this issue.
>
> I would like to mention that I have recently sent out the V4 version of
> the patch series, which addresses the issues related to `cifs_get_tcon_super()`.
> In the latest version, the issue of NULL pointer dereference has already
> been resolved.
>
> https://lore.kernel.org/all/CAH2r5msLMNdqdo6EBuTvrQ0hwrqSRC-LSZuN2WpwV+PkDwsCOw@mail.gmail.com/
>
> I avoid null pointer dereferencing by performing a null pointer check on
> the return value of cifs_get_dfs_tcon_super().
>
>
> > ---
> >   fs/smb/client/misc.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> > index 3b6920a52daa..d73c36862e97 100644
> > --- a/fs/smb/client/misc.c
> > +++ b/fs/smb/client/misc.c
> > @@ -1116,7 +1116,7 @@ static struct super_block *cifs_get_tcon_super(struct cifs_tcon *tcon)
> >       struct cifs_sb_info *cifs_sb;
> >
> >       if (!tcon)
> > -             return NULL;
> > +             return ERR_PTR(-EINVAL);
> >
> >       spin_lock(&tcon->sb_list_lock);
> >       list_for_each_entry(cifs_sb, &tcon->cifs_sb_list, tcon_sb_link) {
> > @@ -1141,7 +1141,7 @@ static struct super_block *cifs_get_tcon_super(struct cifs_tcon *tcon)
> >       }
> >       spin_unlock(&tcon->sb_list_lock);
> >
> > -     return NULL;
> > +     return ERR_PTR(-ENOENT);
> >   }
> >
> >   struct super_block *cifs_get_dfs_tcon_super(struct cifs_tcon *tcon)
>
> Additionally, I think it somewhat peculiar that in the current
> implementation, cifs_get_tcon_super() returns -EINVAL.
>
> I would greatly appreciate it if you could review my latest patch series to
> confirm if it resolves the concerns. If there are any additional improvements, I
> would be happy to collaborate further to ensure the best possible solution.
>
> Best regards,
> Wang Zhaolong
>
>


-- 
Thanks,

Steve

Re: [PATCH next] smb: client: Fix NULL vs ERR_PTR() returns in cifs_get_tcon_super()

[Wang Zhaolong]

> Since Paulo pointed out a problem with v4 of this patch, an obvious
> question is Dan's patch "independent enough" to take or would it make
> the v5 of your patch harder.  Let me know when there is a v5 of the
> patch so we can do more testing and review
> 
> On Mon, Aug 18, 2025 at 8:30 AM Wang Zhaolong
> <wangzhaolong@huaweicloud.com> wrote:
>>
Hi Steve, Paulo, and Dan,

Thank you very much for your attention to this patch.

The NULL pointer issue has already been addressed in the V4
version of the patch. I have considered submitting a V5 version
of the patch, but I have not yet decided whether to proceed with it.

While further analyzing the code, I discovered the following
potential issues in the implementation of `cifs_tree_connect()` in
`dfs.c`, which calls `cifs_get_dfs_tcon_super()`:

1. `cifs_get_dfs_tcon_super()` only returns a single superblock reference.
    Using this superblock, it retrieves the associated `cifs_sb`. However,
    a single tcon is supposed to be associated with multiple `cifs_sb`
    instances. I am unsure whether there is a mechanism that guarantees a
    DFS tcon (where `tcon->origin_fullpath` is non-NULL) has exactly one
    associated `cifs_sb`. My current patch series only retrieves the first
    `cifs_sb` that satisfies the condition, which may not be sufficient.

2. The only purpose of retrieving the `cifs_sb` is to update the `prepath`
    field of the `cifs_sb` in `tree_connect_dfs_target()` by calling
    `cifs_update_super_prepath()`. No other information from the VFS-layer
    superblock is used. However, retrieving the superblock is also meant to
    pin it in memory to prevent it from being released. Therefore, I am
    considering whether to simplify the process of updating the prepath.

If I decide to submit a V5 version of the patch, I will need to fully investigate:
1. Whether a DFS tcon can be associated with multiple `cifs_sb` instances.
2. If multiple `cifs_sb` instances are associated with a DFS tcon, whether
    the `prepath` field of all associated `cifs_sb` instances needs to be
     updated.

Only after thoroughly understanding these two points can I come up with a
better implementation for the V5 patch.

Resolving these questions will take some time. If anyone can provide
guidance or assistance, I would greatly appreciate it.

Best regards,
Wang Zhaolong
Wang Zhaolong

Re: [PATCH next] smb: client: Fix NULL vs ERR_PTR() returns in cifs_get_tcon_super()

[Dan Carpenter]

On Mon, Aug 18, 2025 at 04:41:52PM -0500, Steve French wrote:
> Since Paulo pointed out a problem with v4 of this patch, an obvious
> question is Dan's patch "independent enough" to take or would it make
> the v5 of your patch harder.  Let me know when there is a v5 of the
> patch so we can do more testing and review
> 

Probably it's best to just fix this in version 5 instead of sending a
separate fix patch.

regards,
dan carpenter