* [PATCH 0/1] sget_dev() minor bug fix
@ 2024-04-09 23:31 John Groves
2024-04-09 23:31 ` [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address John Groves
0 siblings, 1 reply; 8+ messages in thread
From: John Groves @ 2024-04-09 23:31 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner, Jan Kara, linux-fsdevel
Cc: John Groves, John Groves, john, 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address
2024-04-09 23:31 [PATCH 0/1] sget_dev() minor bug fix John Groves
@ 2024-04-09 23:31 ` John Groves
2024-04-10 9:18 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: John Groves @ 2024-04-09 23:31 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner, Jan Kara, linux-fsdevel
Cc: John Groves, John Groves, john, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address
2024-04-09 23:31 ` [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address John Groves
@ 2024-04-10 9:18 ` kernel test robot
2024-04-10 10:16 ` Christian Brauner
2024-04-17 5:06 ` kernel test robot
2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-04-10 9:18 UTC (permalink / raw)
To: John Groves, Alexander Viro, Christian Brauner, Jan Kara, linux-fsdevel
Cc: oe-kbuild-all, John Groves, john
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address
2024-04-09 23:31 ` [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address John Groves
2024-04-10 9:18 ` kernel test robot
@ 2024-04-10 10:16 ` Christian Brauner
2024-04-10 13:38 ` John Groves
2024-04-17 5:06 ` kernel test robot
2 siblings, 1 reply; 8+ messages in thread
From: Christian Brauner @ 2024-04-10 10:16 UTC (permalink / raw)
To: John Groves; +Cc: Alexander Viro, Jan Kara, linux-fsdevel, John Groves, john
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address
2024-04-10 10:16 ` Christian Brauner
@ 2024-04-10 13:38 ` John Groves
2024-04-10 15:23 ` Christian Brauner
0 siblings, 1 reply; 8+ messages in thread
From: John Groves @ 2024-04-10 13:38 UTC (permalink / raw)
To: Christian Brauner
Cc: Alexander Viro, Jan Kara, linux-fsdevel, John Groves, john
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address
2024-04-10 13:38 ` John Groves
@ 2024-04-10 15:23 ` Christian Brauner
2024-04-10 21:24 ` John Groves
0 siblings, 1 reply; 8+ messages in thread
From: Christian Brauner @ 2024-04-10 15:23 UTC (permalink / raw)
To: John Groves; +Cc: Alexander Viro, Jan Kara, linux-fsdevel, John Groves, john
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address
2024-04-10 15:23 ` Christian Brauner
@ 2024-04-10 21:24 ` John Groves
0 siblings, 0 replies; 8+ messages in thread
From: John Groves @ 2024-04-10 21:24 UTC (permalink / raw)
To: Christian Brauner
Cc: Alexander Viro, Jan Kara, linux-fsdevel, John Groves, john
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address
2024-04-09 23:31 ` [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address John Groves
2024-04-10 9:18 ` kernel test robot
2024-04-10 10:16 ` Christian Brauner
@ 2024-04-17 5:06 ` kernel test robot
2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-04-17 5:06 UTC (permalink / raw)
To: John Groves
Cc: oe-lkp, lkp, John Groves, linux-fsdevel, Alexander Viro,
Christian Brauner, Jan Kara, John Groves, john, oliver.sang
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-17 5:06 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-09 23:31 [PATCH 0/1] sget_dev() minor bug fix John Groves
2024-04-09 23:31 ` [PATCH 1/1] sget_dev() bug fix: dev_t passed by value but stored via stack address John Groves
2024-04-10 9:18 ` kernel test robot
2024-04-10 10:16 ` Christian Brauner
2024-04-10 13:38 ` John Groves
2024-04-10 15:23 ` Christian Brauner
2024-04-10 21:24 ` John Groves
2024-04-17 5:06 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.