| Message ID | 8ffe993a7aa39881d3e610d5424098ea7ec88180.1652448889.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSv4: Restore nfs4_label into copied nfs_fattr for referrals | expand |
On Fri, 2022-05-13 at 09:36 -0400, Benjamin Coddington wrote: > ..which will fix up trying to free uninitialized nfs4_label: > > PID: 790 TASK: ffff88811b43c000 CPU: 0 COMMAND: "ls" > #0 [ffffc90000857920] panic at ffffffff81b9bfde > #1 [ffffc900008579c0] do_trap at ffffffff81023a9b > #2 [ffffc90000857a10] do_error_trap at ffffffff81023b78 > #3 [ffffc90000857a58] exc_stack_segment at ffffffff81be1f45 > #4 [ffffc90000857a80] asm_exc_stack_segment at ffffffff81c009de > #5 [ffffc90000857b08] nfs_lookup at ffffffffa0302322 [nfs] > #6 [ffffc90000857b70] __lookup_slow at ffffffff813a4a5f > #7 [ffffc90000857c60] walk_component at ffffffff813a86c4 > #8 [ffffc90000857cb8] path_lookupat at ffffffff813a9553 > #9 [ffffc90000857cf0] filename_lookup at ffffffff813ab86b > > Fixes: 9558a007dbc3 ("NFS: Remove the label from the nfs4_lookup_res > struct") > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfs/nfs4proc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index a79f66432bd3..4566280e6ff2 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -4235,6 +4235,7 @@ static int nfs4_get_referral(struct rpc_clnt > *client, struct inode *dir, > int status = -ENOMEM; > struct page *page = NULL; > struct nfs4_fs_locations *locations = NULL; > + struct nfs4_label *label = fattr->label; > > page = alloc_page(GFP_KERNEL); > if (page == NULL) > @@ -4263,6 +4264,7 @@ static int nfs4_get_referral(struct rpc_clnt > *client, struct inode *dir, > > /* replace the lookup nfs_fattr with the locations nfs_fattr > */ > memcpy(fattr, &locations->fattr, sizeof(struct nfs_fattr)); > + fattr->label = label; > memset(fhandle, 0, sizeof(struct nfs_fh)); > out: > if (page) Thanks for finding this, but wouldn't it be better just to decode the fattr in place instead of decoding it into locations->fattr and then doing a memcpy() to get it placed correctly? i.e. add a level of indirection in struct nfs4_fs_locations so that nfs4_xdr_dec_fs_locations() just uses our fattr instead of its own.
On 13 May 2022, at 11:05, Trond Myklebust wrote: > On Fri, 2022-05-13 at 09:36 -0400, Benjamin Coddington wrote: >> ..which will fix up trying to free uninitialized nfs4_label: >> >> PID: 790 TASK: ffff88811b43c000 CPU: 0 COMMAND: "ls" >> #0 [ffffc90000857920] panic at ffffffff81b9bfde >> #1 [ffffc900008579c0] do_trap at ffffffff81023a9b >> #2 [ffffc90000857a10] do_error_trap at ffffffff81023b78 >> #3 [ffffc90000857a58] exc_stack_segment at ffffffff81be1f45 >> #4 [ffffc90000857a80] asm_exc_stack_segment at ffffffff81c009de >> #5 [ffffc90000857b08] nfs_lookup at ffffffffa0302322 [nfs] >> #6 [ffffc90000857b70] __lookup_slow at ffffffff813a4a5f >> #7 [ffffc90000857c60] walk_component at ffffffff813a86c4 >> #8 [ffffc90000857cb8] path_lookupat at ffffffff813a9553 >> #9 [ffffc90000857cf0] filename_lookup at ffffffff813ab86b >> >> Fixes: 9558a007dbc3 ("NFS: Remove the label from the nfs4_lookup_res >> struct") >> Signed-off-by: Benjamin Coddington <bcodding@redhat.com> >> --- >> fs/nfs/nfs4proc.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index a79f66432bd3..4566280e6ff2 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -4235,6 +4235,7 @@ static int nfs4_get_referral(struct rpc_clnt >> *client, struct inode *dir, >> int status = -ENOMEM; >> struct page *page = NULL; >> struct nfs4_fs_locations *locations = NULL; >> + struct nfs4_label *label = fattr->label; >> >> page = alloc_page(GFP_KERNEL); >> if (page == NULL) >> @@ -4263,6 +4264,7 @@ static int nfs4_get_referral(struct rpc_clnt >> *client, struct inode *dir, >> >> /* replace the lookup nfs_fattr with the locations >> nfs_fattr >> */ >> memcpy(fattr, &locations->fattr, sizeof(struct >> nfs_fattr)); >> + fattr->label = label; >> memset(fhandle, 0, sizeof(struct nfs_fh)); >> out: >> if (page) > > Thanks for finding this, but wouldn't it be better just to decode the > fattr in place instead of decoding it into locations->fattr and then > doing a memcpy() to get it placed correctly? > > i.e. add a level of indirection in struct nfs4_fs_locations so that > nfs4_xdr_dec_fs_locations() just uses our fattr instead of its own. Its a much bigger change since we'll end up needing to allocate fattr everywhere fs_locations is used after changing it to a pointer, but if that's the way you want it I'll send it along. Ben
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index a79f66432bd3..4566280e6ff2 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -4235,6 +4235,7 @@ static int nfs4_get_referral(struct rpc_clnt *client, struct inode *dir, int status = -ENOMEM; struct page *page = NULL; struct nfs4_fs_locations *locations = NULL; + struct nfs4_label *label = fattr->label; page = alloc_page(GFP_KERNEL); if (page == NULL) @@ -4263,6 +4264,7 @@ static int nfs4_get_referral(struct rpc_clnt *client, struct inode *dir, /* replace the lookup nfs_fattr with the locations nfs_fattr */ memcpy(fattr, &locations->fattr, sizeof(struct nfs_fattr)); + fattr->label = label; memset(fhandle, 0, sizeof(struct nfs_fh)); out: if (page)
..which will fix up trying to free uninitialized nfs4_label: PID: 790 TASK: ffff88811b43c000 CPU: 0 COMMAND: "ls" #0 [ffffc90000857920] panic at ffffffff81b9bfde #1 [ffffc900008579c0] do_trap at ffffffff81023a9b #2 [ffffc90000857a10] do_error_trap at ffffffff81023b78 #3 [ffffc90000857a58] exc_stack_segment at ffffffff81be1f45 #4 [ffffc90000857a80] asm_exc_stack_segment at ffffffff81c009de #5 [ffffc90000857b08] nfs_lookup at ffffffffa0302322 [nfs] #6 [ffffc90000857b70] __lookup_slow at ffffffff813a4a5f #7 [ffffc90000857c60] walk_component at ffffffff813a86c4 #8 [ffffc90000857cb8] path_lookupat at ffffffff813a9553 #9 [ffffc90000857cf0] filename_lookup at ffffffff813ab86b Fixes: 9558a007dbc3 ("NFS: Remove the label from the nfs4_lookup_res struct") Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/nfs4proc.c | 2 ++ 1 file changed, 2 insertions(+)