diff mbox series

NFSv4: Restore nfs4_label into copied nfs_fattr for referrals

Message ID 8ffe993a7aa39881d3e610d5424098ea7ec88180.1652448889.git.bcodding@redhat.com (mailing list archive)
State New
Headers show
Series NFSv4: Restore nfs4_label into copied nfs_fattr for referrals | expand

Commit Message

Benjamin Coddington May 13, 2022, 1:36 p.m. UTC
..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(+)

Comments

Trond Myklebust May 13, 2022, 3:05 p.m. UTC | #1
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.
Benjamin Coddington May 13, 2022, 6:16 p.m. UTC | #2
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 mbox series

Patch

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)