* [Ocfs2-devel] [PATCH] ocfs2: check if cluster name exists before deref
@ 2014-03-25 17:21 Sasha Levin
2014-03-26 21:33 ` Andrew Morton
2014-03-26 22:00 ` Mark Fasheh
0 siblings, 2 replies; 7+ messages in thread
From: Sasha Levin @ 2014-03-25 17:21 UTC (permalink / raw)
To: ocfs2-devel
Commit c74a3bdd9b "ocfs2: add clustername to cluster connection"
is trying to strlcpy a string which was explicitly passed as NULL
in the very same patch, triggering a NULL ptr deref.
[ 640.225193] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 640.230224] IP: strlcpy (lib/string.c:388 lib/string.c:151)
[ 640.230224] PGD 82a93a067 PUD 82a93b067 PMD 0
[ 640.230224] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 640.230224] Dumping ftrace buffer:
[ 640.230224] (ftrace buffer empty)
[ 640.230224] Modules linked in:
[ 640.230224] CPU: 19 PID: 19426 Comm: trinity-c19 Tainted: G W 3.14.0-rc7-next-20140325-sasha-00014-g9476368-dirty #274
[ 640.230224] task: ffff88082bc53000 ti: ffff88082b674000 task.ti: ffff88082b674000
[ 640.230224] RIP: strlcpy (lib/string.c:388 lib/string.c:151)
[ 640.230224] RSP: 0018:ffff88082b675d88 EFLAGS: 00010296
[ 640.230224] RAX: 0000000000000007 RBX: ffffffff8853b260 RCX: 000000006f6d7366
[ 640.230224] RDX: 0000000000000011 RSI: 0000000000000000 RDI: ffff88052bcd3518
[ 640.230224] RBP: ffff88082b675da8 R08: 00000000746e756f R09: 0000000000000000
[ 640.230224] R10: ffff88052bcd34d0 R11: 0000000000000000 R12: ffff88052bcd3518
[ 640.230224] R13: ffff88052c003fb8 R14: ffff88052bcd34d0 R15: 00000000ffffffea
[ 640.230224] FS: 00007f04ae7a6700(0000) GS:ffff88052cc00000(0000) knlGS:0000000000000000
[ 640.230224] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 640.230224] CR2: 0000000000000000 CR3: 000000082115b000 CR4: 00000000000006a0
[ 640.230224] DR0: 0000000000698000 DR1: 0000000000698000 DR2: 0000000000000000
[ 640.230224] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000602
[ 640.230224] Stack:
[ 640.230224] ffffffff86b3c260 ffffffff8853b260 ffffffff86b3c260 ffff88052c003fb8
[ 640.230224] ffff88082b675df8 ffffffff818a3a5d 0000000000000000 0000000700000000
[ 640.230224] 0000000000000282 ffff88052c003f48 ffff88003e6b01a0 ffff88052c0f81a0
[ 640.230224] Call Trace:
[ 640.230224] ocfs2_cluster_connect (fs/ocfs2/stackglue.c:350)
[ 640.230224] ocfs2_cluster_connect_agnostic (fs/ocfs2/stackglue.c:396)
[ 640.230224] ? ocfs2_control_open (fs/ocfs2/dlmfs/userdlm.c:660)
[ 640.230224] user_dlm_register (fs/ocfs2/dlmfs/userdlm.c:679)
[ 640.230224] ? dlmfs_get_inode (fs/ocfs2/dlmfs/dlmfs.c:468)
[ 640.230224] dlmfs_mkdir (fs/ocfs2/dlmfs/dlmfs.c:503)
[ 640.230224] ? security_inode_permission (security/security.c:555)
[ 640.230224] ? __inode_permission (fs/namei.c:414)
[ 640.230224] vfs_mkdir (fs/namei.c:3467)
[ 640.230224] SyS_mkdirat (fs/namei.c:3488 fs/namei.c:3472)
[ 640.230224] tracesys (arch/x86/kernel/entry_64.S:749)
[ 640.230224] Code: 41 c6 44 1d 00 00 48 83 c4 08 5b 4c 89 e0 41 5c 41 5d 5d c3 0f 1f 80 00 00 00 00 55 48 89 e5 41 55 41 54 49 89 fc 53 48 83 ec 08 <80> 3e 00 74 1c 48 89 f0 0f 1f 84 00 00 00 00 00 48 83 c0 01 80
[ 640.230224] RIP strlcpy (lib/string.c:388 lib/string.c:151)
[ 640.230224] RSP <ffff88082b675d88>
[ 640.230224] CR2: 0000000000000000
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
As a side note, how the hell was this new code path tested?
It's obviously broken and there's no way it even passes
a very basic test.
fs/ocfs2/stackglue.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
index 5e4d314..83f1a66 100644
--- a/fs/ocfs2/stackglue.c
+++ b/fs/ocfs2/stackglue.c
@@ -346,7 +346,9 @@ int ocfs2_cluster_connect(const char *stack_name,
strlcpy(new_conn->cc_name, group, GROUP_NAME_MAX + 1);
new_conn->cc_namelen = grouplen;
- strlcpy(new_conn->cc_cluster_name, cluster_name, CLUSTER_NAME_MAX + 1);
+ if (cluster_name_len)
+ strlcpy(new_conn->cc_cluster_name, cluster_name,
+ CLUSTER_NAME_MAX + 1);
new_conn->cc_cluster_name_len = cluster_name_len;
new_conn->cc_recovery_handler = recovery_handler;
new_conn->cc_recovery_data = recovery_data;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if cluster name exists before deref
2014-03-25 17:21 [Ocfs2-devel] [PATCH] ocfs2: check if cluster name exists before deref Sasha Levin
@ 2014-03-26 21:33 ` Andrew Morton
2014-03-26 21:43 ` Mark Fasheh
2014-03-26 22:00 ` Mark Fasheh
1 sibling, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2014-03-26 21:33 UTC (permalink / raw)
To: ocfs2-devel
On Tue, 25 Mar 2014 13:21:58 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:
> Commit c74a3bdd9b "ocfs2: add clustername to cluster connection"
> is trying to strlcpy a string which was explicitly passed as NULL
> in the very same patch, triggering a NULL ptr deref.
>
> [ 640.225193] BUG: unable to handle kernel NULL pointer dereference at (null)
> [ 640.230224] IP: strlcpy (lib/string.c:388 lib/string.c:151)
Well that was a bit of a screwup.
> As a side note, how the hell was this new code path tested?
> It's obviously broken and there's no way it even passes
> a very basic test.
I was wondering that.
> diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
> index 5e4d314..83f1a66 100644
> --- a/fs/ocfs2/stackglue.c
> +++ b/fs/ocfs2/stackglue.c
> @@ -346,7 +346,9 @@ int ocfs2_cluster_connect(const char *stack_name,
>
> strlcpy(new_conn->cc_name, group, GROUP_NAME_MAX + 1);
> new_conn->cc_namelen = grouplen;
> - strlcpy(new_conn->cc_cluster_name, cluster_name, CLUSTER_NAME_MAX + 1);
> + if (cluster_name_len)
> + strlcpy(new_conn->cc_cluster_name, cluster_name,
> + CLUSTER_NAME_MAX + 1);
> new_conn->cc_cluster_name_len = cluster_name_len;
> new_conn->cc_recovery_handler = recovery_handler;
> new_conn->cc_recovery_data = recovery_data;
So we end up with the null string for the cluster name.
I suppose we can merge this short-term to avoid the oops, but surely
this isn't what's supposed to happen.
Goldwyn, this needs urgent attention please.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if cluster name exists before deref
2014-03-26 21:33 ` Andrew Morton
@ 2014-03-26 21:43 ` Mark Fasheh
0 siblings, 0 replies; 7+ messages in thread
From: Mark Fasheh @ 2014-03-26 21:43 UTC (permalink / raw)
To: ocfs2-devel
CCing Goldywn so he actually gets this e-mail :)
On Wed, Mar 26, 2014 at 02:33:03PM -0700, Andrew Morton wrote:
> On Tue, 25 Mar 2014 13:21:58 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:
>
> > Commit c74a3bdd9b "ocfs2: add clustername to cluster connection"
> > is trying to strlcpy a string which was explicitly passed as NULL
> > in the very same patch, triggering a NULL ptr deref.
> >
> > [ 640.225193] BUG: unable to handle kernel NULL pointer dereference at (null)
> > [ 640.230224] IP: strlcpy (lib/string.c:388 lib/string.c:151)
>
> Well that was a bit of a screwup.
>
> > As a side note, how the hell was this new code path tested?
> > It's obviously broken and there's no way it even passes
> > a very basic test.
>
> I was wondering that.
>
> > diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c
> > index 5e4d314..83f1a66 100644
> > --- a/fs/ocfs2/stackglue.c
> > +++ b/fs/ocfs2/stackglue.c
> > @@ -346,7 +346,9 @@ int ocfs2_cluster_connect(const char *stack_name,
> >
> > strlcpy(new_conn->cc_name, group, GROUP_NAME_MAX + 1);
> > new_conn->cc_namelen = grouplen;
> > - strlcpy(new_conn->cc_cluster_name, cluster_name, CLUSTER_NAME_MAX + 1);
> > + if (cluster_name_len)
> > + strlcpy(new_conn->cc_cluster_name, cluster_name,
> > + CLUSTER_NAME_MAX + 1);
> > new_conn->cc_cluster_name_len = cluster_name_len;
> > new_conn->cc_recovery_handler = recovery_handler;
> > new_conn->cc_recovery_data = recovery_data;
>
> So we end up with the null string for the cluster name.
>
> I suppose we can merge this short-term to avoid the oops, but surely
> this isn't what's supposed to happen.
>
> Goldwyn, this needs urgent attention please.
>
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
--
Mark Fasheh
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if cluster name exists before deref
2014-03-25 17:21 [Ocfs2-devel] [PATCH] ocfs2: check if cluster name exists before deref Sasha Levin
2014-03-26 21:33 ` Andrew Morton
@ 2014-03-26 22:00 ` Mark Fasheh
2014-03-26 22:08 ` Andrew Morton
2014-03-26 22:23 ` Sasha Levin
1 sibling, 2 replies; 7+ messages in thread
From: Mark Fasheh @ 2014-03-26 22:00 UTC (permalink / raw)
To: ocfs2-devel
On Tue, Mar 25, 2014 at 01:21:58PM -0400, Sasha Levin wrote:
> Commit c74a3bdd9b "ocfs2: add clustername to cluster connection"
> is trying to strlcpy a string which was explicitly passed as NULL
> in the very same patch, triggering a NULL ptr deref.
>
> [ 640.225193] BUG: unable to handle kernel NULL pointer dereference at (null)
> [ 640.230224] IP: strlcpy (lib/string.c:388 lib/string.c:151)
> [ 640.230224] PGD 82a93a067 PUD 82a93b067 PMD 0
> [ 640.230224] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
> [ 640.230224] Dumping ftrace buffer:
> [ 640.230224] (ftrace buffer empty)
> [ 640.230224] Modules linked in:
> [ 640.230224] CPU: 19 PID: 19426 Comm: trinity-c19 Tainted: G W 3.14.0-rc7-next-20140325-sasha-00014-g9476368-dirty #274
> [ 640.230224] task: ffff88082bc53000 ti: ffff88082b674000 task.ti: ffff88082b674000
> [ 640.230224] RIP: strlcpy (lib/string.c:388 lib/string.c:151)
> [ 640.230224] RSP: 0018:ffff88082b675d88 EFLAGS: 00010296
> [ 640.230224] RAX: 0000000000000007 RBX: ffffffff8853b260 RCX: 000000006f6d7366
> [ 640.230224] RDX: 0000000000000011 RSI: 0000000000000000 RDI: ffff88052bcd3518
> [ 640.230224] RBP: ffff88082b675da8 R08: 00000000746e756f R09: 0000000000000000
> [ 640.230224] R10: ffff88052bcd34d0 R11: 0000000000000000 R12: ffff88052bcd3518
> [ 640.230224] R13: ffff88052c003fb8 R14: ffff88052bcd34d0 R15: 00000000ffffffea
> [ 640.230224] FS: 00007f04ae7a6700(0000) GS:ffff88052cc00000(0000) knlGS:0000000000000000
> [ 640.230224] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [ 640.230224] CR2: 0000000000000000 CR3: 000000082115b000 CR4: 00000000000006a0
> [ 640.230224] DR0: 0000000000698000 DR1: 0000000000698000 DR2: 0000000000000000
> [ 640.230224] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000602
> [ 640.230224] Stack:
> [ 640.230224] ffffffff86b3c260 ffffffff8853b260 ffffffff86b3c260 ffff88052c003fb8
> [ 640.230224] ffff88082b675df8 ffffffff818a3a5d 0000000000000000 0000000700000000
> [ 640.230224] 0000000000000282 ffff88052c003f48 ffff88003e6b01a0 ffff88052c0f81a0
> [ 640.230224] Call Trace:
> [ 640.230224] ocfs2_cluster_connect (fs/ocfs2/stackglue.c:350)
> [ 640.230224] ocfs2_cluster_connect_agnostic (fs/ocfs2/stackglue.c:396)
> [ 640.230224] ? ocfs2_control_open (fs/ocfs2/dlmfs/userdlm.c:660)
> [ 640.230224] user_dlm_register (fs/ocfs2/dlmfs/userdlm.c:679)
> [ 640.230224] ? dlmfs_get_inode (fs/ocfs2/dlmfs/dlmfs.c:468)
> [ 640.230224] dlmfs_mkdir (fs/ocfs2/dlmfs/dlmfs.c:503)
> [ 640.230224] ? security_inode_permission (security/security.c:555)
> [ 640.230224] ? __inode_permission (fs/namei.c:414)
> [ 640.230224] vfs_mkdir (fs/namei.c:3467)
> [ 640.230224] SyS_mkdirat (fs/namei.c:3488 fs/namei.c:3472)
> [ 640.230224] tracesys (arch/x86/kernel/entry_64.S:749)
> [ 640.230224] Code: 41 c6 44 1d 00 00 48 83 c4 08 5b 4c 89 e0 41 5c 41 5d 5d c3 0f 1f 80 00 00 00 00 55 48 89 e5 41 55 41 54 49 89 fc 53 48 83 ec 08 <80> 3e 00 74 1c 48 89 f0 0f 1f 84 00 00 00 00 00 48 83 c0 01 80
> [ 640.230224] RIP strlcpy (lib/string.c:388 lib/string.c:151)
> [ 640.230224] RSP <ffff88082b675d88>
> [ 640.230224] CR2: 0000000000000000
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
>
> As a side note, how the hell was this new code path tested?
> It's obviously broken and there's no way it even passes
> a very basic test.
I'm not trying to 'defend' Goldwyn, or anything, but mistakes get made -
it's a fact of life. We have a review process to hopefully catch these sorts
of things, you are welcome to take part in it. In fact, if you were
willing to test and report these sorts of things to the Ocfs2-devel list
before they go upstream that would probably help prevent this sort of thing
from happening in the future.
Thanks,
--Mark
--
Mark Fasheh
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if cluster name exists before deref
2014-03-26 22:00 ` Mark Fasheh
@ 2014-03-26 22:08 ` Andrew Morton
2014-03-26 22:23 ` Mark Fasheh
2014-03-26 22:23 ` Sasha Levin
1 sibling, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2014-03-26 22:08 UTC (permalink / raw)
To: ocfs2-devel
On Wed, 26 Mar 2014 15:00:18 -0700 Mark Fasheh <mfasheh@suse.de> wrote:
> > As a side note, how the hell was this new code path tested?
> > It's obviously broken and there's no way it even passes
> > a very basic test.
>
> I'm not trying to 'defend' Goldwyn, or anything, but mistakes get made -
> it's a fact of life. We have a review process to hopefully catch these sorts
> of things, you are welcome to take part in it. In fact, if you were
> willing to test and report these sorts of things to the Ocfs2-devel list
> before they go upstream that would probably help prevent this sort of thing
> from happening in the future.
Well. Sasha is the kernel's reigning testing-and-reporting champion. On
linux-next, no less.
What concerns me is that the wrong patch version was merged, or it got
mangled in flight somehow.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if cluster name exists before deref
2014-03-26 22:08 ` Andrew Morton
@ 2014-03-26 22:23 ` Mark Fasheh
0 siblings, 0 replies; 7+ messages in thread
From: Mark Fasheh @ 2014-03-26 22:23 UTC (permalink / raw)
To: ocfs2-devel
On Wed, Mar 26, 2014 at 03:08:40PM -0700, Andrew Morton wrote:
> On Wed, 26 Mar 2014 15:00:18 -0700 Mark Fasheh <mfasheh@suse.de> wrote:
>
> > > As a side note, how the hell was this new code path tested?
> > > It's obviously broken and there's no way it even passes
> > > a very basic test.
> >
> > I'm not trying to 'defend' Goldwyn, or anything, but mistakes get made -
> > it's a fact of life. We have a review process to hopefully catch these sorts
> > of things, you are welcome to take part in it. In fact, if you were
> > willing to test and report these sorts of things to the Ocfs2-devel list
> > before they go upstream that would probably help prevent this sort of thing
> > from happening in the future.
>
> Well. Sasha is the kernel's reigning testing-and-reporting champion. On
> linux-next, no less.
Huh, Sasha, are you testing Ocfs2 from linux-next? If so some details of how
/ what you test would be appreciated. Otherwise, perhaps we could talk
about some basic Ocfs2 tests then?
> What concerns me is that the wrong patch version was merged, or it got
> mangled in flight somehow.
I kind of assumed that must've been what happened, but probably on
Goldwyn's end - that he sent the wrong version of the patch or something.
It's not like that hasn't happened to me before, etc. In flight would be
weird for me to hear though, I've never experienced that happening.
--Mark
--
Mark Fasheh
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check if cluster name exists before deref
2014-03-26 22:00 ` Mark Fasheh
2014-03-26 22:08 ` Andrew Morton
@ 2014-03-26 22:23 ` Sasha Levin
1 sibling, 0 replies; 7+ messages in thread
From: Sasha Levin @ 2014-03-26 22:23 UTC (permalink / raw)
To: ocfs2-devel
On 03/26/2014 06:00 PM, Mark Fasheh wrote:
> On Tue, Mar 25, 2014 at 01:21:58PM -0400, Sasha Levin wrote:
>> As a side note, how the hell was this new code path tested?
>> It's obviously broken and there's no way it even passes
>> a very basic test.
>
> I'm not trying to 'defend' Goldwyn, or anything, but mistakes get made -
> it's a fact of life. We have a review process to hopefully catch these sorts
> of things, you are welcome to take part in it. In fact, if you were
> willing to test and report these sorts of things to the Ocfs2-devel list
> before they go upstream that would probably help prevent this sort of thing
> from happening in the future.
Hi Mark,
I'm fuzz testing the kernel and not really testing or reviewing any subsystem
in particular.
My frustration about not testing it was because it's so simple to reproduce this
issue:
# mount -t ocfs2_dlmfs ocfs2_dlmfs dlm
# mkdir dlm/anything
[ boom ]
This is a very basic test that should have been run more than once before this
patch made it into Linus's tree. Whether as part of some ocfs2 test suite or
maybe xfstest.
My beef is not with Goldwyn not testing that path, it's more with whatever
process (or lack thereof) ocfs2 has to test and verify patches before
they make it upstream. Even if Goldwyn missed it, it should have been
caught at a different place on it's way.
Thanks,
Sasha
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-03-26 22:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-25 17:21 [Ocfs2-devel] [PATCH] ocfs2: check if cluster name exists before deref Sasha Levin
2014-03-26 21:33 ` Andrew Morton
2014-03-26 21:43 ` Mark Fasheh
2014-03-26 22:00 ` Mark Fasheh
2014-03-26 22:08 ` Andrew Morton
2014-03-26 22:23 ` Mark Fasheh
2014-03-26 22:23 ` Sasha Levin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).