[PATCH 0/1] sget_dev() minor bug fix

[PATCH 0/1] sget_dev() minor bug fix

[John Groves]

I found this small mess while working on applying Christian Brauner's
very helpful suggestions for get_tree() handling in famfs (for which
v2 is coming soon).

John Groves (1):
  sget_dev() bug fix: dev_t passed by value but stored via stack address

 fs/super.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)


base-commit: fec50db7033ea478773b159e0e2efb135270e3b7
-- 
2.43.0

[PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address

[John Groves]

The ref vs. value logic used by sget_dev() was ungood, storing the
stack address of the key (dev_t) rather than the value of the key.
This straightens that out.

In the sget_dev() path, the (void *)data passed to the test and set
helpers should be the value of the dev_t, not its address.

Signed-off-by: John Groves <john@groves.net>
---
 fs/super.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 69ce6c600968..b4ef775e95da 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1308,7 +1308,9 @@ EXPORT_SYMBOL(get_tree_keyed);
 
 static int set_bdev_super(struct super_block *s, void *data)
 {
-	s->s_dev = *(dev_t *)data;
+	u64 devno = (u64)data;
+
+	s->s_dev = (dev_t)devno;
 	return 0;
 }
 
@@ -1319,8 +1321,10 @@ static int super_s_dev_set(struct super_block *s, struct fs_context *fc)
 
 static int super_s_dev_test(struct super_block *s, struct fs_context *fc)
 {
+	u64 devno = (u64)fc->sget_key;
+
 	return !(s->s_iflags & SB_I_RETIRED) &&
-		s->s_dev == *(dev_t *)fc->sget_key;
+		s->s_dev == (dev_t)devno;
 }
 
 /**
@@ -1345,7 +1349,9 @@ static int super_s_dev_test(struct super_block *s, struct fs_context *fc)
  */
 struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
 {
-	fc->sget_key = &dev;
+	u64 devno = (u64)dev;
+
+	fc->sget_key = (void *)devno;
 	return sget_fc(fc, super_s_dev_test, super_s_dev_set);
 }
 EXPORT_SYMBOL(sget_dev);
@@ -1637,13 +1643,15 @@ struct dentry *mount_bdev(struct file_system_type *fs_type,
 	struct super_block *s;
 	int error;
 	dev_t dev;
+	u64 devno;
 
 	error = lookup_bdev(dev_name, &dev);
 	if (error)
 		return ERR_PTR(error);
 
+	devno = (u64)dev;
 	flags |= SB_NOSEC;
-	s = sget(fs_type, test_bdev_super, set_bdev_super, flags, &dev);
+	s = sget(fs_type, test_bdev_super, set_bdev_super, flags, (void *)devno);
 	if (IS_ERR(s))
 		return ERR_CAST(s);
 
-- 
2.43.0

Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address

[kernel test robot]

Hi John,

kernel test robot noticed the following build warnings:

[auto build test WARNING on fec50db7033ea478773b159e0e2efb135270e3b7]

url:    https://github.com/intel-lab-lkp/linux/commits/John-Groves/sget_dev-bug-fix-dev_t-passed-by-value-but-stored-via-stack-address/20240410-073305
base:   fec50db7033ea478773b159e0e2efb135270e3b7
patch link:    https://lore.kernel.org/r/7a37d4832e0c2e7cfe8000b0bf47dcc2c50d78d0.1712704849.git.john%40groves.net
patch subject: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address
config: openrisc-allnoconfig (https://download.01.org/0day-ci/archive/20240410/202404101632.62NM3BlE-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240410/202404101632.62NM3BlE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404101632.62NM3BlE-lkp@intel.com/

All warnings (new ones prefixed by >>):

   fs/super.c: In function 'set_bdev_super':
>> fs/super.c:1311:21: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1311 |         u64 devno = (u64)data;
         |                     ^
   fs/super.c: In function 'super_s_dev_test':
   fs/super.c:1324:21: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1324 |         u64 devno = (u64)fc->sget_key;
         |                     ^
   fs/super.c: In function 'sget_dev':
>> fs/super.c:1354:24: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    1354 |         fc->sget_key = (void *)devno;
         |                        ^
   fs/super.c: In function 'mount_bdev':
   fs/super.c:1654:67: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    1654 |         s = sget(fs_type, test_bdev_super, set_bdev_super, flags, (void *)devno);
         |                                                                   ^


vim +1311 fs/super.c

  1308	
  1309	static int set_bdev_super(struct super_block *s, void *data)
  1310	{
> 1311		u64 devno = (u64)data;
  1312	
  1313		s->s_dev = (dev_t)devno;
  1314		return 0;
  1315	}
  1316	
  1317	static int super_s_dev_set(struct super_block *s, struct fs_context *fc)
  1318	{
  1319		return set_bdev_super(s, fc->sget_key);
  1320	}
  1321	
  1322	static int super_s_dev_test(struct super_block *s, struct fs_context *fc)
  1323	{
  1324		u64 devno = (u64)fc->sget_key;
  1325	
  1326		return !(s->s_iflags & SB_I_RETIRED) &&
  1327			s->s_dev == (dev_t)devno;
  1328	}
  1329	
  1330	/**
  1331	 * sget_dev - Find or create a superblock by device number
  1332	 * @fc: Filesystem context.
  1333	 * @dev: device number
  1334	 *
  1335	 * Find or create a superblock using the provided device number that
  1336	 * will be stored in fc->sget_key.
  1337	 *
  1338	 * If an extant superblock is matched, then that will be returned with
  1339	 * an elevated reference count that the caller must transfer or discard.
  1340	 *
  1341	 * If no match is made, a new superblock will be allocated and basic
  1342	 * initialisation will be performed (s_type, s_fs_info, s_id, s_dev will
  1343	 * be set). The superblock will be published and it will be returned in
  1344	 * a partially constructed state with SB_BORN and SB_ACTIVE as yet
  1345	 * unset.
  1346	 *
  1347	 * Return: an existing or newly created superblock on success, an error
  1348	 *         pointer on failure.
  1349	 */
  1350	struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
  1351	{
  1352		u64 devno = (u64)dev;
  1353	
> 1354		fc->sget_key = (void *)devno;
  1355		return sget_fc(fc, super_s_dev_test, super_s_dev_set);
  1356	}
  1357	EXPORT_SYMBOL(sget_dev);
  1358	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address

[Christian Brauner]

On Tue, Apr 09, 2024 at 06:31:44PM -0500, John Groves wrote:
> The ref vs. value logic used by sget_dev() was ungood, storing the
> stack address of the key (dev_t) rather than the value of the key.
> This straightens that out.
> 
> In the sget_dev() path, the (void *)data passed to the test and set
> helpers should be the value of the dev_t, not its address.
> 
> Signed-off-by: John Groves <john@groves.net>
> ---

Afaict there's nothing wrong with the current logic so I'm missing your
point here. It's casting to a dev_t and then dereferencing it. So I
don't think this patch makes sense.

Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address

[John Groves]

On 24/04/10 12:16PM, Christian Brauner wrote:
> On Tue, Apr 09, 2024 at 06:31:44PM -0500, John Groves wrote:
> > The ref vs. value logic used by sget_dev() was ungood, storing the
> > stack address of the key (dev_t) rather than the value of the key.
> > This straightens that out.
> > 
> > In the sget_dev() path, the (void *)data passed to the test and set
> > helpers should be the value of the dev_t, not its address.
> > 
> > Signed-off-by: John Groves <john@groves.net>
> > ---
> 
> Afaict there's nothing wrong with the current logic so I'm missing your
> point here. It's casting to a dev_t and then dereferencing it. So I
> don't think this patch makes sense.

Hi Christian,

Apologies, I got confused myself and fubar'd this.

But I believe there is at least one actual problem; please correct
me if I'm wrong, and thanks for your patience if so.

In sget_dev() - original here:

	struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
	{
		fc->sget_key = &dev;
		return sget_fc(fc, super_s_dev_test, super_s_dev_set);
	}

I don't think &dev makes sense here - it was passed by value so its
address won't make sense outside the current context, right?. It seems
like it should be:

	fc->sget_key = (void *)dev;

But that assumes we're using the value of sget_key rather than what
it points to, which I now see is not the case - super_s_dev_test()
is testing for (s->s_dev == *(dev_t *)fc->sget_key), so that wants
sget_key to be a pointer to a dev_t.

But I don't see anywhere that sget_key points to something that was
allocated; the dev_t for sget_dev() appears to be the only user and it's
not something whose address can validly be stored in and dereferenced
from a pointer later (am I wrong?!).

I looked at this because I tried to use sget_dev() in famfs, for which 
it seems to do the right thing (although the dev_t is character in 
the famfs case). But it never found the existing superblock even when 
I knew it existed - so I dug myself a little hole ;)

Although my hacks went off the rails, I was sort-of trying to make
sget_key a value rather than a pointer, because the pointer thing 
didn't make sense to me (at least for the dev_t case). It seems like 
that should be a value unless you have other uses in mind that need 
it to actually point somewhere.

I can try again with this additional clarity, but the "key" question
is whether sget_key really needs to be a pointer - which depends on
what else you want to use it for. Type checking would certainly be
easier if it wasn't a void *...

Thank you,
John

Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address

[Christian Brauner]

> I don't think &dev makes sense here - it was passed by value so its
> address won't make sense outside the current context, right?. It seems

I don't follow, we only need that address to be valid until sget_dev()
returns as sget_key isn't used anymore. And we store the value, not the
address. Other than that it's a bit ugly it's fine afaict. Related
issues would exist with fuse or gfs2 where the lifetime of the key ends
right after the respective sget call returns. We could smooth this out
here by storing the value in the pointer via
#define devt_to_sget_key(dev) ((void *)((uintptr_t)(dev)))
#define sget_key_to_devt(key) ((dev_t)((uintptr_t)(key)))
but I'm not sure it's necessary. Unless I'm really missing something.

Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address

[John Groves]

On 24/04/10 05:23PM, Christian Brauner wrote:
> > I don't think &dev makes sense here - it was passed by value so its
> > address won't make sense outside the current context, right?. It seems
> 
> I don't follow, we only need that address to be valid until sget_dev()
> returns as sget_key isn't used anymore. And we store the value, not the
> address. Other than that it's a bit ugly it's fine afaict. Related
> issues would exist with fuse or gfs2 where the lifetime of the key ends
> right after the respective sget call returns. We could smooth this out
> here by storing the value in the pointer via
> #define devt_to_sget_key(dev) ((void *)((uintptr_t)(dev)))
> #define sget_key_to_devt(key) ((dev_t)((uintptr_t)(key)))

That's the gist of what my patch was trying to do, but it was messy 
because void * is 8 bytes and dev_t is 4 bytes, so casting got a bit 
messy (not even including my fubar-ness)..

Not that my vote is important, but I would heartily vote for storing the
dev_t in the sget_key rather than having the key be a void * to a 
soon-to-be-reused stack. Future developers would have an easier 
time with that, if I'm any indication.

> but I'm not sure it's necessary. Unless I'm really missing something.

I must admit it hadn't even occurred to me that a void * to a dev_t
would be stored long-term in fs_context even though it is only short-
term valid. But you're right, that works provided the validity lifespan 
isn't violated - which it isn't here.

I was around in the K&R C time frame, and... well, never mind...

Thanks for your patience with me on this. I guess my sget_dev() was 
failing to find my old superblocks for a different reason - like I 
thought they still existed but was mistaken. You may see a Q or two 
from me on the old famfs thread soon, prior to v2 of that...

Thank you,
John

Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address

[kernel test robot]

Hello,

kernel test robot noticed "canonical_address#:#[##]" on:

commit: 22170ab79e39d9675bf9aa8d8e08c28759e14533 ("[PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address")
url: https://github.com/intel-lab-lkp/linux/commits/John-Groves/sget_dev-bug-fix-dev_t-passed-by-value-but-stored-via-stack-address/20240410-073305
patch link: https://lore.kernel.org/all/7a37d4832e0c2e7cfe8000b0bf47dcc2c50d78d0.1712704849.git.john@groves.net/
patch subject: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address

in testcase: xfstests
version: xfstests-x86_64-e72e052d-1_20240415
with following parameters:

	disk: 4HDD
	fs: f2fs
	test: generic-group-15



compiler: gcc-13
test machine: 8 threads Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz (Skylake) with 28G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202404171222.628d7c98-oliver.sang@intel.com



[   42.142026][ T1384] general protection fault, probably for non-canonical address 0xdffffc0000100000: 0000 [#1] PREEMPT SMP KASAN PTI
[   42.153892][ T1384] KASAN: probably user-memory-access in range [0x0000000000800000-0x0000000000800007]
[   42.163241][ T1384] CPU: 1 PID: 1384 Comm: mount Tainted: G S                 6.9.0-rc3-00001-g22170ab79e39 #1
[   42.173196][ T1384] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.2.8 01/26/2016
[ 42.181248][ T1384] RIP: 0010:test_bdev_super (kbuild/src/consumer/fs/super.c:1636 (discriminator 1)) 
[ 42.186453][ T1384] Code: 8d 7b 10 48 89 fa 48 c1 ea 03 0f b6 04 02 84 c0 74 04 3c 03 7e 3a 48 b8 00 00 00 00 00 fc ff df 48 89 f2 8b 5b 10 48 c1 ea 03 <0f> b6 14 02 48 89 f0 83 e0 07 83 c0 03 38 d0 7c 04 84 d2 75 20 31
All code
========
   0:	8d 7b 10             	lea    0x10(%rbx),%edi
   3:	48 89 fa             	mov    %rdi,%rdx
   6:	48 c1 ea 03          	shr    $0x3,%rdx
   a:	0f b6 04 02          	movzbl (%rdx,%rax,1),%eax
   e:	84 c0                	test   %al,%al
  10:	74 04                	je     0x16
  12:	3c 03                	cmp    $0x3,%al
  14:	7e 3a                	jle    0x50
  16:	48 b8 00 00 00 00 00 	movabs $0xdffffc0000000000,%rax
  1d:	fc ff df 
  20:	48 89 f2             	mov    %rsi,%rdx
  23:	8b 5b 10             	mov    0x10(%rbx),%ebx
  26:	48 c1 ea 03          	shr    $0x3,%rdx
  2a:*	0f b6 14 02          	movzbl (%rdx,%rax,1),%edx		<-- trapping instruction
  2e:	48 89 f0             	mov    %rsi,%rax
  31:	83 e0 07             	and    $0x7,%eax
  34:	83 c0 03             	add    $0x3,%eax
  37:	38 d0                	cmp    %dl,%al
  39:	7c 04                	jl     0x3f
  3b:	84 d2                	test   %dl,%dl
  3d:	75 20                	jne    0x5f
  3f:	31                   	.byte 0x31

Code starting with the faulting instruction
===========================================
   0:	0f b6 14 02          	movzbl (%rdx,%rax,1),%edx
   4:	48 89 f0             	mov    %rsi,%rax
   7:	83 e0 07             	and    $0x7,%eax
   a:	83 c0 03             	add    $0x3,%eax
   d:	38 d0                	cmp    %dl,%al
   f:	7c 04                	jl     0x15
  11:	84 d2                	test   %dl,%dl
  13:	75 20                	jne    0x35
  15:	31                   	.byte 0x31
[   42.205827][ T1384] RSP: 0018:ffffc90000e1fba8 EFLAGS: 00010206
[   42.211722][ T1384] RAX: dffffc0000000000 RBX: 0000000000800001 RCX: ffffffff83bf0f85
[   42.219517][ T1384] RDX: 0000000000100000 RSI: 0000000000800002 RDI: ffff888161ef1010
[   42.227310][ T1384] RBP: ffffffffc1739580 R08: 0000000000000001 R09: fffff520001c3f6d
[   42.235103][ T1384] R10: 0000000000000003 R11: ffffffff85fecd94 R12: ffffffff81b413b0
[   42.242896][ T1384] R13: ffffffff84b229a0 R14: 0000000000800002 R15: ffff888161ef1000
[   42.250692][ T1384] FS:  00007fed4c83a840(0000) GS:ffff888635080000(0000) knlGS:0000000000000000
[   42.259435][ T1384] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   42.265846][ T1384] CR2: 0000560fcf853018 CR3: 000000074c9ac001 CR4: 00000000003706f0
[   42.273641][ T1384] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   42.281432][ T1384] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   42.289224][ T1384] Call Trace:
[   42.292350][ T1384]  <TASK>
[ 42.295131][ T1384] ? die_addr (kbuild/src/consumer/arch/x86/kernel/dumpstack.c:421 kbuild/src/consumer/arch/x86/kernel/dumpstack.c:460) 
[ 42.299126][ T1384] ? exc_general_protection (kbuild/src/consumer/arch/x86/kernel/traps.c:702 kbuild/src/consumer/arch/x86/kernel/traps.c:644) 
[ 42.304501][ T1384] ? asm_exc_general_protection (kbuild/src/consumer/arch/x86/include/asm/idtentry.h:617) 
[ 42.310049][ T1384] ? __pfx_test_bdev_super (kbuild/src/consumer/fs/super.c:1635) 


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240417/202404171222.628d7c98-oliver.sang@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki