* [PATCH] cifs: missing null pointer check in cifs_mount
@ 2021-06-23 1:17 Steve French
2021-06-23 11:48 ` Aurélien Aptel
0 siblings, 1 reply; 12+ messages in thread
From: Steve French @ 2021-06-23 1:17 UTC (permalink / raw)
To: CIFS; +Cc: Paulo Alcantara, ronnie sahlberg
[-- Attachment #1: Type: text/plain, Size: 755 bytes --]
We weren't checking if tcon is null before setting dfs path,
although we check for null tcon in an earlier assignment statement.
Addresses-Coverity: 1476411 ("Dereference after null check")
Signed-off-by: Steve French <stfrench@microsoft.com>
---
fs/cifs/connect.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 8d95607a9312..196ef9ec69db 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3480,7 +3480,8 @@ int cifs_mount(struct cifs_sb_info *cifs_sb,
struct smb3_fs_context *ctx)
goto error;
}
spin_lock(&cifs_tcp_ses_lock);
- tcon->dfs_path = ref_path;
+ if (tcon)
+ tcon->dfs_path = ref_path;
ref_path = NULL;
spin_unlock(&cifs_tcp_ses_lock);
--
Thanks,
Steve
[-- Attachment #2: 0001-cifs-missing-null-pointer-check-in-cifs_mount.patch --]
[-- Type: text/x-patch, Size: 968 bytes --]
From 632096b66b2fa2621e3d2d02c2c2fd436975810b Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 22 Jun 2021 20:13:44 -0500
Subject: [PATCH] cifs: missing null pointer check in cifs_mount
We weren't checking if tcon is null before setting dfs path,
although we check for null tcon in an earlier assignment statement.
Addresses-Coverity: 1476411 ("Dereference after null check")
Signed-off-by: Steve French <stfrench@microsoft.com>
---
fs/cifs/connect.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 8d95607a9312..196ef9ec69db 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3480,7 +3480,8 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
goto error;
}
spin_lock(&cifs_tcp_ses_lock);
- tcon->dfs_path = ref_path;
+ if (tcon)
+ tcon->dfs_path = ref_path;
ref_path = NULL;
spin_unlock(&cifs_tcp_ses_lock);
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2021-06-23 1:17 [PATCH] cifs: missing null pointer check in cifs_mount Steve French
@ 2021-06-23 11:48 ` Aurélien Aptel
2021-06-23 12:17 ` Paulo Alcantara
0 siblings, 1 reply; 12+ messages in thread
From: Aurélien Aptel @ 2021-06-23 11:48 UTC (permalink / raw)
To: Steve French, CIFS; +Cc: Paulo Alcantara, ronnie sahlberg
Steve French <smfrench@gmail.com> writes:
> We weren't checking if tcon is null before setting dfs path,
> although we check for null tcon in an earlier assignment statement.
If tcon is NULL there is no point in continuing in that function, we
should have exited earlier.
If tcon is NULL it means mount_get_conns() failed so presumably rc will
be != 0 and we would goto error.
I don't think this is needed. We could change the existing check after
the loop to this you really want to be safe:
if (rc || !tcon)
goto error;
Cheers,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2021-06-23 11:48 ` Aurélien Aptel
@ 2021-06-23 12:17 ` Paulo Alcantara
2021-06-24 0:34 ` Steve French
0 siblings, 1 reply; 12+ messages in thread
From: Paulo Alcantara @ 2021-06-23 12:17 UTC (permalink / raw)
To: Aurélien Aptel, Steve French, CIFS; +Cc: ronnie sahlberg
Agreed.
On June 23, 2021 8:48:24 AM GMT-03:00, "Aurélien Aptel" <aaptel@suse.com> wrote:
>Steve French <smfrench@gmail.com> writes:
>> We weren't checking if tcon is null before setting dfs path,
>> although we check for null tcon in an earlier assignment statement.
>
>If tcon is NULL there is no point in continuing in that function, we
>should have exited earlier.
>
>If tcon is NULL it means mount_get_conns() failed so presumably rc will
>be != 0 and we would goto error.
>
>I don't think this is needed. We could change the existing check after
>the loop to this you really want to be safe:
>
> if (rc || !tcon)
> goto error;
>
>
>Cheers,
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2021-06-23 12:17 ` Paulo Alcantara
@ 2021-06-24 0:34 ` Steve French
2023-08-11 13:16 ` Jeff Layton
0 siblings, 1 reply; 12+ messages in thread
From: Steve French @ 2021-06-24 0:34 UTC (permalink / raw)
To: Paulo Alcantara; +Cc: Aurélien Aptel, CIFS, ronnie sahlberg
[-- Attachment #1: Type: text/plain, Size: 883 bytes --]
updated patch attached with Aurelien's suggestion.
On Wed, Jun 23, 2021 at 7:17 AM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Agreed.
>
> On June 23, 2021 8:48:24 AM GMT-03:00, "Aurélien Aptel" <aaptel@suse.com> wrote:
> >Steve French <smfrench@gmail.com> writes:
> >> We weren't checking if tcon is null before setting dfs path,
> >> although we check for null tcon in an earlier assignment statement.
> >
> >If tcon is NULL there is no point in continuing in that function, we
> >should have exited earlier.
> >
> >If tcon is NULL it means mount_get_conns() failed so presumably rc will
> >be != 0 and we would goto error.
> >
> >I don't think this is needed. We could change the existing check after
> >the loop to this you really want to be safe:
> >
> > if (rc || !tcon)
> > goto error;
> >
> >
> >Cheers,
--
Thanks,
Steve
[-- Attachment #2: 0001-cifs-missing-null-pointer-check-in-cifs_mount.patch --]
[-- Type: text/x-patch, Size: 899 bytes --]
From 162004a2f7ef5c77600e364dc4e9315b0e6ca386 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Wed, 23 Jun 2021 19:32:24 -0500
Subject: [PATCH] cifs: missing null pointer check in cifs_mount
We weren't checking if tcon is null before setting dfs path,
although we check for null tcon in an earlier assignment statement.
Addresses-Coverity: 1476411 ("Dereference after null check")
Signed-off-by: Steve French <stfrench@microsoft.com>
---
fs/cifs/connect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 8d95607a9312..c8079376d294 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3451,7 +3451,7 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
rc = -ELOOP;
} while (rc == -EREMOTE);
- if (rc)
+ if (rc || !tcon)
goto error;
kfree(ref_path);
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2021-06-24 0:34 ` Steve French
@ 2023-08-11 13:16 ` Jeff Layton
2023-08-11 15:15 ` Paulo Alcantara
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2023-08-11 13:16 UTC (permalink / raw)
To: Steve French, Paulo Alcantara
Cc: Aurélien Aptel, CIFS, ronnie sahlberg, Benjamin Coddington,
Jay Shin
On Wed, 2021-06-23 at 19:34 -0500, Steve French wrote:
> updated patch attached with Aurelien's suggestion.
>
> On Wed, Jun 23, 2021 at 7:17 AM Paulo Alcantara <pc@cjr.nz> wrote:
> >
> > Agreed.
> >
> > On June 23, 2021 8:48:24 AM GMT-03:00, "Aurélien Aptel" <aaptel@suse.com> wrote:
> > > Steve French <smfrench@gmail.com> writes:
> > > > We weren't checking if tcon is null before setting dfs path,
> > > > although we check for null tcon in an earlier assignment statement.
> > >
> > > If tcon is NULL there is no point in continuing in that function, we
> > > should have exited earlier.
> > >
> > > If tcon is NULL it means mount_get_conns() failed so presumably rc will
> > > be != 0 and we would goto error.
> > >
> > > I don't think this is needed. We could change the existing check after
> > > the loop to this you really want to be safe:
> > >
> > > if (rc || !tcon)
> > > goto error;
> > >
> > >
> > > Cheers,
>
>
>
I know this patch is ancient and the mainline code has marched on, but
it seems really suspicious to me.
With this, we have cifs_mount returning 0, even though the superblock
hasn't been properly initialized. Is that expected? Shouldn't it return
an error in that case?
The mount handling has morphed considerably since this patch went in, so
I can't really tell whether this was later fixed or not.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2023-08-11 13:16 ` Jeff Layton
@ 2023-08-11 15:15 ` Paulo Alcantara
2023-08-11 16:26 ` Jeff Layton
0 siblings, 1 reply; 12+ messages in thread
From: Paulo Alcantara @ 2023-08-11 15:15 UTC (permalink / raw)
To: Jeff Layton, Steve French
Cc: Aurélien Aptel, CIFS, ronnie sahlberg, Benjamin Coddington,
Jay Shin
Jeff Layton <jlayton@kernel.org> writes:
> On Wed, 2021-06-23 at 19:34 -0500, Steve French wrote:
>> updated patch attached with Aurelien's suggestion.
>>
>> On Wed, Jun 23, 2021 at 7:17 AM Paulo Alcantara <pc@cjr.nz> wrote:
>> >
>> > Agreed.
>> >
>> > On June 23, 2021 8:48:24 AM GMT-03:00, "Aurélien Aptel" <aaptel@suse.com> wrote:
>> > > Steve French <smfrench@gmail.com> writes:
>> > > > We weren't checking if tcon is null before setting dfs path,
>> > > > although we check for null tcon in an earlier assignment statement.
>> > >
>> > > If tcon is NULL there is no point in continuing in that function, we
>> > > should have exited earlier.
>> > >
>> > > If tcon is NULL it means mount_get_conns() failed so presumably rc will
>> > > be != 0 and we would goto error.
>> > >
>> > > I don't think this is needed. We could change the existing check after
>> > > the loop to this you really want to be safe:
>> > >
>> > > if (rc || !tcon)
>> > > goto error;
>> > >
>> > >
>> > > Cheers,
>>
>>
>>
>
> I know this patch is ancient and the mainline code has marched on, but
> it seems really suspicious to me.
Yes, it is.
> With this, we have cifs_mount returning 0, even though the superblock
> hasn't been properly initialized. Is that expected? Shouldn't it return
> an error in that case?
No, that isn't expected. And yes, if @tcon would ever be NULL at that
point, we should be returning an error instead. Otherwise we'd end up
dereferencing a NULL @tcon while trying to get an inode for the root
dentry later.
However, by quickly looking at the old code -- on top of 162004a2f7ef --
I don't see how we'd end up having a NULL @tcon with rc == 0 as
mount_get_conns() would return -errno if it couldn't get a tcon. Please
correct me if I'm missing something. Whether it is possibile or not,
the NULL @tcon check is certainly missing a 'rc = -ENOENT' or some other
error before bailing out as you've pointed out.
> The mount handling has morphed considerably since this patch went in, so
> I can't really tell whether this was later fixed or not.
I don't think there was a follow-up patch for that.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2023-08-11 15:15 ` Paulo Alcantara
@ 2023-08-11 16:26 ` Jeff Layton
2023-08-11 16:49 ` Paulo Alcantara
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2023-08-11 16:26 UTC (permalink / raw)
To: Paulo Alcantara, Steve French
Cc: Aurélien Aptel, CIFS, ronnie sahlberg, Benjamin Coddington,
Jay Shin, Roberto Bergantinos Corpas
On Fri, 2023-08-11 at 12:15 -0300, Paulo Alcantara wrote:
> Jeff Layton <jlayton@kernel.org> writes:
>
> > On Wed, 2021-06-23 at 19:34 -0500, Steve French wrote:
> > > updated patch attached with Aurelien's suggestion.
> > >
> > > On Wed, Jun 23, 2021 at 7:17 AM Paulo Alcantara <pc@cjr.nz> wrote:
> > > >
> > > > Agreed.
> > > >
> > > > On June 23, 2021 8:48:24 AM GMT-03:00, "Aurélien Aptel" <aaptel@suse.com> wrote:
> > > > > Steve French <smfrench@gmail.com> writes:
> > > > > > We weren't checking if tcon is null before setting dfs path,
> > > > > > although we check for null tcon in an earlier assignment statement.
> > > > >
> > > > > If tcon is NULL there is no point in continuing in that function, we
> > > > > should have exited earlier.
> > > > >
> > > > > If tcon is NULL it means mount_get_conns() failed so presumably rc will
> > > > > be != 0 and we would goto error.
> > > > >
> > > > > I don't think this is needed. We could change the existing check after
> > > > > the loop to this you really want to be safe:
> > > > >
> > > > > if (rc || !tcon)
> > > > > goto error;
> > > > >
> > > > >
> > > > > Cheers,
> > >
> > >
> > >
> >
> > I know this patch is ancient and the mainline code has marched on, but
> > it seems really suspicious to me.
>
> Yes, it is.
>
> > With this, we have cifs_mount returning 0, even though the superblock
> > hasn't been properly initialized. Is that expected? Shouldn't it return
> > an error in that case?
>
> No, that isn't expected. And yes, if @tcon would ever be NULL at that
> point, we should be returning an error instead. Otherwise we'd end up
> dereferencing a NULL @tcon while trying to get an inode for the root
> dentry later.
>
> However, by quickly looking at the old code -- on top of 162004a2f7ef --
> I don't see how we'd end up having a NULL @tcon with rc == 0 as
> mount_get_conns() would return -errno if it couldn't get a tcon. Please
> correct me if I'm missing something. Whether it is possibile or not,
> the NULL @tcon check is certainly missing a 'rc = -ENOENT' or some other
> error before bailing out as you've pointed out.
Thanks for the confirmation. There were some oopses on some RHEL8 (5.14
based kernels). The stack looked something like this:
PID: 2415716 TASK: ffff937139090000 CPU: 3 COMMAND: "ls"
#0 [ffff9ef946b23728] machine_kexec at ffffffffac867cfe
#1 [ffff9ef946b23780] __crash_kexec at ffffffffac9ad94d
#2 [ffff9ef946b23848] crash_kexec at ffffffffac9ae881
#3 [ffff9ef946b23860] oops_end at ffffffffac8274f1
#4 [ffff9ef946b23880] no_context at ffffffffac879a03
#5 [ffff9ef946b238d8] __bad_area_nosemaphore at ffffffffac879d64
#6 [ffff9ef946b23920] do_page_fault at ffffffffac87a617
#7 [ffff9ef946b23950] page_fault at ffffffffad20111e
[exception RIP: cifs_mount+1126]
RIP: ffffffffc08e8826 RSP: ffff9ef946b23a00 RFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff936c8221ea00 RCX: ffff936f8018b320
RDX: 0000000000000001 RSI: ffff936f8018b420 RDI: ffff936c8221ea00
RBP: ffff9ef946b23a90 R8: 5346445756535c5c R9: 6765642e50313031
R10: 65622e666f6f7267 R11: 0063696c6275505c R12: ffff9370b192fc00
R13: ffff936f8018b420 R14: 00000000003097ad R15: 0000000000000000
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
#8 [ffff9ef946b23a98] cifs_smb3_do_mount at ffffffffc08d11f2 [cifs]
#9 [ffff9ef946b23aa0] cifs_smb3_do_mount at ffffffffc08d11f2 [cifs]
#10 [ffff9ef946b23b08] smb3_get_tree at ffffffffc0930ae0 [cifs]
#11 [ffff9ef946b23b30] vfs_get_tree at ffffffffacb52365
#12 [ffff9ef946b23b50] fc_mount at ffffffffacb7485e
#13 [ffff9ef946b23b60] vfs_kern_mount at ffffffffacb748ec
#14 [ffff9ef946b23b80] cifs_dfs_do_automount at ffffffffc093552e [cifs]
#15 [ffff9ef946b23bc0] cifs_dfs_d_automount at ffffffffc0935880 [cifs]
#16 [ffff9ef946b23bd0] follow_managed at ffffffffacb5bdaf
#17 [ffff9ef946b23c10] lookup_fast at ffffffffacb5c7e5
#18 [ffff9ef946b23c68] walk_component at ffffffffacb5d258
#19 [ffff9ef946b23cc8] path_lookupat at ffffffffacb5e215
#20 [ffff9ef946b23d28] filename_lookup at ffffffffacb62710
#21 [ffff9ef946b23e40] vfs_statx at ffffffffacb55874
#22 [ffff9ef946b23e98] __do_sys_statx at ffffffffacb5692b
#23 [ffff9ef946b23f38] do_syscall_64 at ffffffffac8043ab
#24 [ffff9ef946b23f50] entry_SYSCALL_64_after_hwframe at
ffffffffad2000a9
RIP: 00007ff6a2637edf RSP: 00007ffe040017d0 RFLAGS: 00000246
RAX: ffffffffffffffda RBX: 00007ffe04001910 RCX: 00007ff6a2637edf
RDX: 0000000000000100 RSI: 00007ffe04001910 RDI: 00000000ffffff9c
RBP: 0000000000000100 R8: 00007ffe040017f0 R9: 00000000ffffff9c
R10: 0000000000000002 R11: 0000000000000246 R12: 00007ffe040017f0
R13: 0000000000000000 R14: 0000000000000003 R15: 000055ce1a3ae1b8
ORIG_RAX: 000000000000014c CS: 0033 SS: 002b
Analysis of the vmcore by Roberto showed that we had ended up past that
point with tcon==NULL and rc==0.
Steve's patch would have fixed the panic there, but I think the host
would have ended up with a successful mount, but with a broken
superblock. The current code seems a bit less fragile, and I didn't see
any similar brokenness there, but I didn't look too hard either.
In any case, we'll plan to fix this up with a one-off in RHEL/Centos.
Thanks again for the sanity check!
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2023-08-11 16:26 ` Jeff Layton
@ 2023-08-11 16:49 ` Paulo Alcantara
2023-08-11 16:58 ` Jeff Layton
0 siblings, 1 reply; 12+ messages in thread
From: Paulo Alcantara @ 2023-08-11 16:49 UTC (permalink / raw)
To: Jeff Layton, Steve French
Cc: Aurélien Aptel, CIFS, ronnie sahlberg, Benjamin Coddington,
Jay Shin, Roberto Bergantinos Corpas
Jeff Layton <jlayton@kernel.org> writes:
> Thanks for the confirmation. There were some oopses on some RHEL8 (5.14
> based kernels). The stack looked something like this:
>
> PID: 2415716 TASK: ffff937139090000 CPU: 3 COMMAND: "ls"
> #0 [ffff9ef946b23728] machine_kexec at ffffffffac867cfe
> #1 [ffff9ef946b23780] __crash_kexec at ffffffffac9ad94d
> #2 [ffff9ef946b23848] crash_kexec at ffffffffac9ae881
> #3 [ffff9ef946b23860] oops_end at ffffffffac8274f1
> #4 [ffff9ef946b23880] no_context at ffffffffac879a03
> #5 [ffff9ef946b238d8] __bad_area_nosemaphore at ffffffffac879d64
> #6 [ffff9ef946b23920] do_page_fault at ffffffffac87a617
> #7 [ffff9ef946b23950] page_fault at ffffffffad20111e
> [exception RIP: cifs_mount+1126]
> RIP: ffffffffc08e8826 RSP: ffff9ef946b23a00 RFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff936c8221ea00 RCX: ffff936f8018b320
> RDX: 0000000000000001 RSI: ffff936f8018b420 RDI: ffff936c8221ea00
> RBP: ffff9ef946b23a90 R8: 5346445756535c5c R9: 6765642e50313031
> R10: 65622e666f6f7267 R11: 0063696c6275505c R12: ffff9370b192fc00
> R13: ffff936f8018b420 R14: 00000000003097ad R15: 0000000000000000
> ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> #8 [ffff9ef946b23a98] cifs_smb3_do_mount at ffffffffc08d11f2 [cifs]
> #9 [ffff9ef946b23aa0] cifs_smb3_do_mount at ffffffffc08d11f2 [cifs]
> #10 [ffff9ef946b23b08] smb3_get_tree at ffffffffc0930ae0 [cifs]
> #11 [ffff9ef946b23b30] vfs_get_tree at ffffffffacb52365
> #12 [ffff9ef946b23b50] fc_mount at ffffffffacb7485e
> #13 [ffff9ef946b23b60] vfs_kern_mount at ffffffffacb748ec
> #14 [ffff9ef946b23b80] cifs_dfs_do_automount at ffffffffc093552e [cifs]
> #15 [ffff9ef946b23bc0] cifs_dfs_d_automount at ffffffffc0935880 [cifs]
> #16 [ffff9ef946b23bd0] follow_managed at ffffffffacb5bdaf
> #17 [ffff9ef946b23c10] lookup_fast at ffffffffacb5c7e5
> #18 [ffff9ef946b23c68] walk_component at ffffffffacb5d258
> #19 [ffff9ef946b23cc8] path_lookupat at ffffffffacb5e215
> #20 [ffff9ef946b23d28] filename_lookup at ffffffffacb62710
> #21 [ffff9ef946b23e40] vfs_statx at ffffffffacb55874
> #22 [ffff9ef946b23e98] __do_sys_statx at ffffffffacb5692b
> #23 [ffff9ef946b23f38] do_syscall_64 at ffffffffac8043ab
> #24 [ffff9ef946b23f50] entry_SYSCALL_64_after_hwframe at
> ffffffffad2000a9
> RIP: 00007ff6a2637edf RSP: 00007ffe040017d0 RFLAGS: 00000246
> RAX: ffffffffffffffda RBX: 00007ffe04001910 RCX: 00007ff6a2637edf
> RDX: 0000000000000100 RSI: 00007ffe04001910 RDI: 00000000ffffff9c
> RBP: 0000000000000100 R8: 00007ffe040017f0 R9: 00000000ffffff9c
> R10: 0000000000000002 R11: 0000000000000246 R12: 00007ffe040017f0
> R13: 0000000000000000 R14: 0000000000000003 R15: 000055ce1a3ae1b8
> ORIG_RAX: 000000000000014c CS: 0033 SS: 002b
>
> Analysis of the vmcore by Roberto showed that we had ended up past that
> point with tcon==NULL and rc==0.
Interesting. Thanks for sharing the backtrace!
So it actually ended up with NULL @tcon and rc == 0 while mounting a DFS
link. Nice catch!
> Steve's patch would have fixed the panic there, but I think the host
> would have ended up with a successful mount, but with a broken
> superblock. The current code seems a bit less fragile, and I didn't see
> any similar brokenness there, but I didn't look too hard either.
Yeah, makes sense.
> In any case, we'll plan to fix this up with a one-off in RHEL/Centos.
> Thanks again for the sanity check!
Would you mind to propose a patch that fixes the above and mark it for
v5.14..v5.15?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2023-08-11 16:49 ` Paulo Alcantara
@ 2023-08-11 16:58 ` Jeff Layton
2023-08-11 17:06 ` Paulo Alcantara
2023-08-15 14:25 ` Jeff Layton
0 siblings, 2 replies; 12+ messages in thread
From: Jeff Layton @ 2023-08-11 16:58 UTC (permalink / raw)
To: Paulo Alcantara, Steve French
Cc: Aurélien Aptel, CIFS, ronnie sahlberg, Benjamin Coddington,
Jay Shin, Roberto Bergantinos Corpas
On Fri, 2023-08-11 at 13:49 -0300, Paulo Alcantara wrote:
> Jeff Layton <jlayton@kernel.org> writes:
>
> > Thanks for the confirmation. There were some oopses on some RHEL8 (5.14
> > based kernels). The stack looked something like this:
> >
> > PID: 2415716 TASK: ffff937139090000 CPU: 3 COMMAND: "ls"
> > #0 [ffff9ef946b23728] machine_kexec at ffffffffac867cfe
> > #1 [ffff9ef946b23780] __crash_kexec at ffffffffac9ad94d
> > #2 [ffff9ef946b23848] crash_kexec at ffffffffac9ae881
> > #3 [ffff9ef946b23860] oops_end at ffffffffac8274f1
> > #4 [ffff9ef946b23880] no_context at ffffffffac879a03
> > #5 [ffff9ef946b238d8] __bad_area_nosemaphore at ffffffffac879d64
> > #6 [ffff9ef946b23920] do_page_fault at ffffffffac87a617
> > #7 [ffff9ef946b23950] page_fault at ffffffffad20111e
> > [exception RIP: cifs_mount+1126]
> > RIP: ffffffffc08e8826 RSP: ffff9ef946b23a00 RFLAGS: 00010246
> > RAX: 0000000000000000 RBX: ffff936c8221ea00 RCX: ffff936f8018b320
> > RDX: 0000000000000001 RSI: ffff936f8018b420 RDI: ffff936c8221ea00
> > RBP: ffff9ef946b23a90 R8: 5346445756535c5c R9: 6765642e50313031
> > R10: 65622e666f6f7267 R11: 0063696c6275505c R12: ffff9370b192fc00
> > R13: ffff936f8018b420 R14: 00000000003097ad R15: 0000000000000000
> > ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> > #8 [ffff9ef946b23a98] cifs_smb3_do_mount at ffffffffc08d11f2 [cifs]
> > #9 [ffff9ef946b23aa0] cifs_smb3_do_mount at ffffffffc08d11f2 [cifs]
> > #10 [ffff9ef946b23b08] smb3_get_tree at ffffffffc0930ae0 [cifs]
> > #11 [ffff9ef946b23b30] vfs_get_tree at ffffffffacb52365
> > #12 [ffff9ef946b23b50] fc_mount at ffffffffacb7485e
> > #13 [ffff9ef946b23b60] vfs_kern_mount at ffffffffacb748ec
> > #14 [ffff9ef946b23b80] cifs_dfs_do_automount at ffffffffc093552e [cifs]
> > #15 [ffff9ef946b23bc0] cifs_dfs_d_automount at ffffffffc0935880 [cifs]
> > #16 [ffff9ef946b23bd0] follow_managed at ffffffffacb5bdaf
> > #17 [ffff9ef946b23c10] lookup_fast at ffffffffacb5c7e5
> > #18 [ffff9ef946b23c68] walk_component at ffffffffacb5d258
> > #19 [ffff9ef946b23cc8] path_lookupat at ffffffffacb5e215
> > #20 [ffff9ef946b23d28] filename_lookup at ffffffffacb62710
> > #21 [ffff9ef946b23e40] vfs_statx at ffffffffacb55874
> > #22 [ffff9ef946b23e98] __do_sys_statx at ffffffffacb5692b
> > #23 [ffff9ef946b23f38] do_syscall_64 at ffffffffac8043ab
> > #24 [ffff9ef946b23f50] entry_SYSCALL_64_after_hwframe at
> > ffffffffad2000a9
> > RIP: 00007ff6a2637edf RSP: 00007ffe040017d0 RFLAGS: 00000246
> > RAX: ffffffffffffffda RBX: 00007ffe04001910 RCX: 00007ff6a2637edf
> > RDX: 0000000000000100 RSI: 00007ffe04001910 RDI: 00000000ffffff9c
> > RBP: 0000000000000100 R8: 00007ffe040017f0 R9: 00000000ffffff9c
> > R10: 0000000000000002 R11: 0000000000000246 R12: 00007ffe040017f0
> > R13: 0000000000000000 R14: 0000000000000003 R15: 000055ce1a3ae1b8
> > ORIG_RAX: 000000000000014c CS: 0033 SS: 002b
> >
> > Analysis of the vmcore by Roberto showed that we had ended up past that
> > point with tcon==NULL and rc==0.
>
> Interesting. Thanks for sharing the backtrace!
>
> So it actually ended up with NULL @tcon and rc == 0 while mounting a DFS
> link. Nice catch!
>
> > Steve's patch would have fixed the panic there, but I think the host
> > would have ended up with a successful mount, but with a broken
> > superblock. The current code seems a bit less fragile, and I didn't see
> > any similar brokenness there, but I didn't look too hard either.
>
> Yeah, makes sense.
>
> > In any case, we'll plan to fix this up with a one-off in RHEL/Centos.
> > Thanks again for the sanity check!
>
> Would you mind to propose a patch that fixes the above and mark it for
> v5.14..v5.15?
Sounds good. One of us will make sure that happens too.
Thanks!
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2023-08-11 16:58 ` Jeff Layton
@ 2023-08-11 17:06 ` Paulo Alcantara
2023-08-15 14:25 ` Jeff Layton
1 sibling, 0 replies; 12+ messages in thread
From: Paulo Alcantara @ 2023-08-11 17:06 UTC (permalink / raw)
To: Jeff Layton, Steve French
Cc: Aurélien Aptel, CIFS, ronnie sahlberg, Benjamin Coddington,
Jay Shin, Roberto Bergantinos Corpas
Jeff Layton <jlayton@kernel.org> writes:
> On Fri, 2023-08-11 at 13:49 -0300, Paulo Alcantara wrote:
>> > In any case, we'll plan to fix this up with a one-off in RHEL/Centos.
>> > Thanks again for the sanity check!
>>
>> Would you mind to propose a patch that fixes the above and mark it for
>> v5.14..v5.15?
>
> Sounds good. One of us will make sure that happens too.
Thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2023-08-11 16:58 ` Jeff Layton
2023-08-11 17:06 ` Paulo Alcantara
@ 2023-08-15 14:25 ` Jeff Layton
2023-08-15 18:35 ` Paulo Alcantara
1 sibling, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2023-08-15 14:25 UTC (permalink / raw)
To: Paulo Alcantara, Steve French
Cc: Aurélien Aptel, CIFS, ronnie sahlberg, Benjamin Coddington,
Jay Shin, Roberto Bergantinos Corpas
[-- Attachment #1: Type: text/plain, Size: 4402 bytes --]
On Fri, 2023-08-11 at 12:58 -0400, Jeff Layton wrote:
> On Fri, 2023-08-11 at 13:49 -0300, Paulo Alcantara wrote:
> > Jeff Layton <jlayton@kernel.org> writes:
> >
> > > Thanks for the confirmation. There were some oopses on some RHEL8 (5.14
> > > based kernels). The stack looked something like this:
> > >
> > > PID: 2415716 TASK: ffff937139090000 CPU: 3 COMMAND: "ls"
> > > #0 [ffff9ef946b23728] machine_kexec at ffffffffac867cfe
> > > #1 [ffff9ef946b23780] __crash_kexec at ffffffffac9ad94d
> > > #2 [ffff9ef946b23848] crash_kexec at ffffffffac9ae881
> > > #3 [ffff9ef946b23860] oops_end at ffffffffac8274f1
> > > #4 [ffff9ef946b23880] no_context at ffffffffac879a03
> > > #5 [ffff9ef946b238d8] __bad_area_nosemaphore at ffffffffac879d64
> > > #6 [ffff9ef946b23920] do_page_fault at ffffffffac87a617
> > > #7 [ffff9ef946b23950] page_fault at ffffffffad20111e
> > > [exception RIP: cifs_mount+1126]
> > > RIP: ffffffffc08e8826 RSP: ffff9ef946b23a00 RFLAGS: 00010246
> > > RAX: 0000000000000000 RBX: ffff936c8221ea00 RCX: ffff936f8018b320
> > > RDX: 0000000000000001 RSI: ffff936f8018b420 RDI: ffff936c8221ea00
> > > RBP: ffff9ef946b23a90 R8: 5346445756535c5c R9: 6765642e50313031
> > > R10: 65622e666f6f7267 R11: 0063696c6275505c R12: ffff9370b192fc00
> > > R13: ffff936f8018b420 R14: 00000000003097ad R15: 0000000000000000
> > > ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> > > #8 [ffff9ef946b23a98] cifs_smb3_do_mount at ffffffffc08d11f2 [cifs]
> > > #9 [ffff9ef946b23aa0] cifs_smb3_do_mount at ffffffffc08d11f2 [cifs]
> > > #10 [ffff9ef946b23b08] smb3_get_tree at ffffffffc0930ae0 [cifs]
> > > #11 [ffff9ef946b23b30] vfs_get_tree at ffffffffacb52365
> > > #12 [ffff9ef946b23b50] fc_mount at ffffffffacb7485e
> > > #13 [ffff9ef946b23b60] vfs_kern_mount at ffffffffacb748ec
> > > #14 [ffff9ef946b23b80] cifs_dfs_do_automount at ffffffffc093552e [cifs]
> > > #15 [ffff9ef946b23bc0] cifs_dfs_d_automount at ffffffffc0935880 [cifs]
> > > #16 [ffff9ef946b23bd0] follow_managed at ffffffffacb5bdaf
> > > #17 [ffff9ef946b23c10] lookup_fast at ffffffffacb5c7e5
> > > #18 [ffff9ef946b23c68] walk_component at ffffffffacb5d258
> > > #19 [ffff9ef946b23cc8] path_lookupat at ffffffffacb5e215
> > > #20 [ffff9ef946b23d28] filename_lookup at ffffffffacb62710
> > > #21 [ffff9ef946b23e40] vfs_statx at ffffffffacb55874
> > > #22 [ffff9ef946b23e98] __do_sys_statx at ffffffffacb5692b
> > > #23 [ffff9ef946b23f38] do_syscall_64 at ffffffffac8043ab
> > > #24 [ffff9ef946b23f50] entry_SYSCALL_64_after_hwframe at
> > > ffffffffad2000a9
> > > RIP: 00007ff6a2637edf RSP: 00007ffe040017d0 RFLAGS: 00000246
> > > RAX: ffffffffffffffda RBX: 00007ffe04001910 RCX: 00007ff6a2637edf
> > > RDX: 0000000000000100 RSI: 00007ffe04001910 RDI: 00000000ffffff9c
> > > RBP: 0000000000000100 R8: 00007ffe040017f0 R9: 00000000ffffff9c
> > > R10: 0000000000000002 R11: 0000000000000246 R12: 00007ffe040017f0
> > > R13: 0000000000000000 R14: 0000000000000003 R15: 000055ce1a3ae1b8
> > > ORIG_RAX: 000000000000014c CS: 0033 SS: 002b
> > >
> > > Analysis of the vmcore by Roberto showed that we had ended up past that
> > > point with tcon==NULL and rc==0.
> >
> > Interesting. Thanks for sharing the backtrace!
> >
> > So it actually ended up with NULL @tcon and rc == 0 while mounting a DFS
> > link. Nice catch!
> >
> > > Steve's patch would have fixed the panic there, but I think the host
> > > would have ended up with a successful mount, but with a broken
> > > superblock. The current code seems a bit less fragile, and I didn't see
> > > any similar brokenness there, but I didn't look too hard either.
> >
> > Yeah, makes sense.
> >
> > > In any case, we'll plan to fix this up with a one-off in RHEL/Centos.
> > > Thanks again for the sanity check!
> >
> > Would you mind to propose a patch that fixes the above and mark it for
> > v5.14..v5.15?
>
> Sounds good. One of us will make sure that happens too.
>
FWIW, I took a look at v5.15.125 and I don't see the same bug there. It
probably got fixed inadvertently with some other backporting. Looks like
this is only a problem for older, non-stable-series kernels.
The patch I created for RHEL8 is attached though, if you're interested.
--
Jeff Layton <jlayton@kernel.org>
[-- Attachment #2: 0001-cifs-fix-bogus-cifs_mount-error-handling-in-RHEL8.patch --]
[-- Type: text/x-patch, Size: 1028 bytes --]
From 9d97e2c37e98e893e38b3e64923ca68dc9c21e61 Mon Sep 17 00:00:00 2001
From: Jeffrey Layton <jlayton@redhat.com>
Date: Tue, 15 Aug 2023 09:42:21 -0400
Subject: [PATCH] cifs: fix bogus cifs_mount error handling in RHEL8
The patch that originally went in for this bug was bogus and could cause
cifs_mount to return 0, even though it hadn't properly filled out the
superblock.
Ensure that we also return an error in this case. Paulo Alcantara
suggested -ENOENT in the upstream discussion.
Signed-off-by: Jeffrey Layton <jlayton@redhat.com>
---
fs/cifs/connect.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index f882b38d4ed3..0af0c0718153 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3507,8 +3507,10 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
rc = -ELOOP;
} while (rc == -EREMOTE);
- if (rc || !tcon)
+ if (rc || !tcon) {
+ rc = rc ? rc : -ENOENT;
goto error;
+ }
kfree(ref_path);
/*
--
2.41.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] cifs: missing null pointer check in cifs_mount
2023-08-15 14:25 ` Jeff Layton
@ 2023-08-15 18:35 ` Paulo Alcantara
0 siblings, 0 replies; 12+ messages in thread
From: Paulo Alcantara @ 2023-08-15 18:35 UTC (permalink / raw)
To: Jeff Layton, Steve French
Cc: Aurélien Aptel, CIFS, ronnie sahlberg, Benjamin Coddington,
Jay Shin, Roberto Bergantinos Corpas
Jeff Layton <jlayton@kernel.org> writes:
> FWIW, I took a look at v5.15.125 and I don't see the same bug there. It
> probably got fixed inadvertently with some other backporting. Looks like
> this is only a problem for older, non-stable-series kernels.
Thanks for looking into that! Really appreciate it.
> The patch I created for RHEL8 is attached though, if you're
> interested.
LGTM.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-08-15 18:36 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-23 1:17 [PATCH] cifs: missing null pointer check in cifs_mount Steve French
2021-06-23 11:48 ` Aurélien Aptel
2021-06-23 12:17 ` Paulo Alcantara
2021-06-24 0:34 ` Steve French
2023-08-11 13:16 ` Jeff Layton
2023-08-11 15:15 ` Paulo Alcantara
2023-08-11 16:26 ` Jeff Layton
2023-08-11 16:49 ` Paulo Alcantara
2023-08-11 16:58 ` Jeff Layton
2023-08-11 17:06 ` Paulo Alcantara
2023-08-15 14:25 ` Jeff Layton
2023-08-15 18:35 ` Paulo Alcantara
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.