diff mbox series

ceph: fix variable dereferenced before check in ceph_umount_begin()

Message ID 20250328183359.1101617-1-slava@dubeyko.com (mailing list archive)
State New
Headers show
Series ceph: fix variable dereferenced before check in ceph_umount_begin() | expand

Commit Message

Viacheslav Dubeyko March 28, 2025, 6:33 p.m. UTC 
From: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

smatch warnings:
fs/ceph/super.c:1042 ceph_umount_begin() warn: variable dereferenced before check 'fsc' (see line 1041)

vim +/fsc +1042 fs/ceph/super.c

void ceph_umount_begin(struct super_block *sb)
{
	struct ceph_fs_client *fsc = ceph_sb_to_fs_client(sb);

	doutc(fsc->client, "starting forced umount\n");
              ^^^^^^^^^^^
Dereferenced

	if (!fsc)
            ^^^^
Checked too late.

		return;
	fsc->mount_state = CEPH_MOUNT_SHUTDOWN;
	__ceph_umount_begin(fsc);
}

This patch moves pointer check before the first
dereference of the pointer.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_r_202503280852.YDB3pxUY-2Dlkp-40intel.com_&d=DwIBAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=Ud7uNdqBY_Z7LJ_oI4fwdhvxOYt_5Q58tpkMQgDWhV3199_TCnINFU28Esc0BaAH&s=QOKWZ9HKLyd6XCxW-AUoKiFFg9roId6LOM01202zAk0&e=
Signed-off-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
---
 fs/ceph/super.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Matthew Wilcox March 28, 2025, 6:43 p.m. UTC | #1 
On Fri, Mar 28, 2025 at 11:33:59AM -0700, Viacheslav Dubeyko wrote:
> This patch moves pointer check before the first
> dereference of the pointer.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_r_202503280852.YDB3pxUY-2Dlkp-40intel.com_&d=DwIBAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=Ud7uNdqBY_Z7LJ_oI4fwdhvxOYt_5Q58tpkMQgDWhV3199_TCnINFU28Esc0BaAH&s=QOKWZ9HKLyd6XCxW-AUoKiFFg9roId6LOM01202zAk0&e=

Ooh, that's not good.  Need to figure out a way to defeat the proofpoint
garbage.

> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index f3951253e393..6cbc33c56e0e 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -1032,9 +1032,11 @@ void ceph_umount_begin(struct super_block *sb)
>  {
>  	struct ceph_fs_client *fsc = ceph_sb_to_fs_client(sb);
>  
> -	doutc(fsc->client, "starting forced umount\n");
>  	if (!fsc)
>  		return;
> +
> +	doutc(fsc->client, "starting forced umount\n");

I don't think we should be checking fsc against NULL.  I don't see a way
that sb->s_fs_info can be set to NULL, do you?
Viacheslav Dubeyko March 28, 2025, 7:30 p.m. UTC | #2 
On Fri, 2025-03-28 at 18:43 +0000, Matthew Wilcox wrote:
> On Fri, Mar 28, 2025 at 11:33:59AM -0700, Viacheslav Dubeyko wrote:
> > This patch moves pointer check before the first
> > dereference of the pointer.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202503280852.YDB3pxUY-lkp@intel.com/ 
> 
> Ooh, that's not good.  Need to figure out a way to defeat the proofpoint
> garbage.
> 

Yeah, this is not good.

> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index f3951253e393..6cbc33c56e0e 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -1032,9 +1032,11 @@ void ceph_umount_begin(struct super_block *sb)
> >  {
> >  	struct ceph_fs_client *fsc = ceph_sb_to_fs_client(sb);
> >  
> > -	doutc(fsc->client, "starting forced umount\n");
> >  	if (!fsc)
> >  		return;
> > +
> > +	doutc(fsc->client, "starting forced umount\n");
> 
> I don't think we should be checking fsc against NULL.  I don't see a way
> that sb->s_fs_info can be set to NULL, do you?

I assume because forced umount could happen anytime, potentially, we could have
sb->s_fs_info not set. But, frankly speaking, I started to worry about fsc-
>client:

struct ceph_fs_client {
	struct super_block *sb;

	struct list_head metric_wakeup;

	struct ceph_mount_options *mount_options;
	struct ceph_client *client;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
        We are trying to access the client pointer in debug output. 

<skipped>
};

So, maybe, we need to add fsc->client check here too.

Thanks,
Slava.
Dan Carpenter March 31, 2025, 8:23 a.m. UTC | #3 
On Fri, Mar 28, 2025 at 06:43:36PM +0000, Matthew Wilcox wrote:
> On Fri, Mar 28, 2025 at 11:33:59AM -0700, Viacheslav Dubeyko wrote:
> > This patch moves pointer check before the first
> > dereference of the pointer.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_r_202503280852.YDB3pxUY-2Dlkp-40intel.com_&d=DwIBAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=Ud7uNdqBY_Z7LJ_oI4fwdhvxOYt_5Q58tpkMQgDWhV3199_TCnINFU28Esc0BaAH&s=QOKWZ9HKLyd6XCxW-AUoKiFFg9roId6LOM01202zAk0&e=
> 
> Ooh, that's not good.  Need to figure out a way to defeat the proofpoint
> garbage.

Option 1 for procmail users:
https://github.com/wjshamblin/proofpoint_rewrite

Option 2: Copy the tag from lore.
https://lore.kernel.org/all/?q=ceph_umount_begin

regards,
dan carpenter
Christian Brauner April 1, 2025, 10:38 a.m. UTC | #4 
On Fri, Mar 28, 2025 at 07:30:11PM +0000, Viacheslav Dubeyko wrote:
> On Fri, 2025-03-28 at 18:43 +0000, Matthew Wilcox wrote:
> > On Fri, Mar 28, 2025 at 11:33:59AM -0700, Viacheslav Dubeyko wrote:
> > > This patch moves pointer check before the first
> > > dereference of the pointer.
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Closes: https://lore.kernel.org/r/202503280852.YDB3pxUY-lkp@intel.com/ 
> > 
> > Ooh, that's not good.  Need to figure out a way to defeat the proofpoint
> > garbage.
> > 
> 
> Yeah, this is not good.
> 
> > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > index f3951253e393..6cbc33c56e0e 100644
> > > --- a/fs/ceph/super.c
> > > +++ b/fs/ceph/super.c
> > > @@ -1032,9 +1032,11 @@ void ceph_umount_begin(struct super_block *sb)
> > >  {
> > >  	struct ceph_fs_client *fsc = ceph_sb_to_fs_client(sb);
> > >  
> > > -	doutc(fsc->client, "starting forced umount\n");
> > >  	if (!fsc)
> > >  		return;
> > > +
> > > +	doutc(fsc->client, "starting forced umount\n");
> > 
> > I don't think we should be checking fsc against NULL.  I don't see a way
> > that sb->s_fs_info can be set to NULL, do you?
> 
> I assume because forced umount could happen anytime, potentially, we could have
> sb->s_fs_info not set. But, frankly speaking, I started to worry about fsc-

No, it must be set. The VFS guarantees that the superblock is still
alive when it calls into ceph via ->umount_begin().
Viacheslav Dubeyko April 1, 2025, 6:29 p.m. UTC | #5 
On Tue, 2025-04-01 at 12:38 +0200, Christian Brauner wrote:
> On Fri, Mar 28, 2025 at 07:30:11PM +0000, Viacheslav Dubeyko wrote:
> > On Fri, 2025-03-28 at 18:43 +0000, Matthew Wilcox wrote:
> > > On Fri, Mar 28, 2025 at 11:33:59AM -0700, Viacheslav Dubeyko wrote:
> > > > This patch moves pointer check before the first
> > > > dereference of the pointer.
> > > > 
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > Closes: https://lore.kernel.org/r/202503280852.YDB3pxUY-lkp@intel.com/   
> > > 
> > > Ooh, that's not good.  Need to figure out a way to defeat the proofpoint
> > > garbage.
> > > 
> > 
> > Yeah, this is not good.
> > 
> > > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > > index f3951253e393..6cbc33c56e0e 100644
> > > > --- a/fs/ceph/super.c
> > > > +++ b/fs/ceph/super.c
> > > > @@ -1032,9 +1032,11 @@ void ceph_umount_begin(struct super_block *sb)
> > > >  {
> > > >  	struct ceph_fs_client *fsc = ceph_sb_to_fs_client(sb);
> > > >  
> > > > -	doutc(fsc->client, "starting forced umount\n");
> > > >  	if (!fsc)
> > > >  		return;
> > > > +
> > > > +	doutc(fsc->client, "starting forced umount\n");
> > > 
> > > I don't think we should be checking fsc against NULL.  I don't see a way
> > > that sb->s_fs_info can be set to NULL, do you?
> > 
> > I assume because forced umount could happen anytime, potentially, we could have
> > sb->s_fs_info not set. But, frankly speaking, I started to worry about fsc-
> 
> No, it must be set. The VFS guarantees that the superblock is still
> alive when it calls into ceph via ->umount_begin().

So, if we have the guarantee of fsc pointer validity, then we need to change
this checking of fsc->client pointer. Or, probably, completely remove this check
here?

Thanks,
Slava.
diff mbox series

Patch

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index f3951253e393..6cbc33c56e0e 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -1032,9 +1032,11 @@  void ceph_umount_begin(struct super_block *sb)
 {
 	struct ceph_fs_client *fsc = ceph_sb_to_fs_client(sb);
 
-	doutc(fsc->client, "starting forced umount\n");
 	if (!fsc)
 		return;
+
+	doutc(fsc->client, "starting forced umount\n");
+
 	fsc->mount_state = CEPH_MOUNT_SHUTDOWN;
 	__ceph_umount_begin(fsc);
 }