Re: [PATCH V3] nfsdcld: fix cld pipe read size

["zhangjian (CG)" <zhangjian496@huawei.com>]

It's really that you say. I will fix it at once.

On 2025/2/28 5:25, Scott Mayhew wrote:
> On Thu, 27 Feb 2025, zhangjian wrote:
> 
>> When nfsd inits failed for detecting cld version in
>> nfsd4_client_tracking_init, kernel may assume nfsdcld support version 1
>> message format and try to upcall with v1 message size to nfsdcld.
>> There exists one error case in the following process, causeing nfsd
>> hunging for nfsdcld replay:
>>
>> kernel write to pipe->msgs (v1 msg length)
>>     |--------- first msg --------|-------- second message -------|
>>
>> nfsdcld read from pipe->msgs (v2 msg length)
>>     |------------ first msg --------------|---second message-----|
>>     |  valid message             | ignore |     wrong message    |
>>
>> When two nfsd kernel thread add two upcall messages to cld pipe with v1
>> version cld_msg (size == 1034) concurrently，but nfsdcld reads with v2
>> version size(size == 1067), 33 bytes of the second message will be read
>> and merged with first message. The 33 bytes in second message will be
>> ignored. Nfsdcld will then read 1001 bytes in second message, which cause
>> FATAL in cld_messaged_size checking. Nfsd kernel thread will hang for
>> it forever until nfs server restarts.
>>
>> Signed-off-by: zhangjian <zhangjian496@huawei.com>
>> Reviewed-by: Scott Mayhew <smayhew@redhat.com>
>> ---
>>  utils/nfsdcld/nfsdcld.c | 65 ++++++++++++++++++++++++++++-------------
>>  1 file changed, 45 insertions(+), 20 deletions(-)
>>
>> diff --git a/utils/nfsdcld/nfsdcld.c b/utils/nfsdcld/nfsdcld.c
>> index dbc7a57..005d1ea 100644
>> --- a/utils/nfsdcld/nfsdcld.c
>> +++ b/utils/nfsdcld/nfsdcld.c
>> @@ -716,35 +716,60 @@ reply:
>>  	}
>>  }
>>  
>> -static void
>> -cldcb(int UNUSED(fd), short which, void *data)
>> +static int
>> +cld_pipe_read_msg(struct cld_client *clnt)
>>  {
>> -	ssize_t len;
>> -	struct cld_client *clnt = data;
>> -#if UPCALL_VERSION >= 2
>> -	struct cld_msg_v2 *cmsg = &clnt->cl_u.cl_msg_v2;
>> -#else
>> -	struct cld_msg *cmsg = &clnt->cl_u.cl_msg;
>> -#endif
>> +	ssize_t len, left_len;
>> +	ssize_t hdr_len = sizeof(struct cld_msg_hdr);
>> +	struct cld_msg_hdr *hdr = (struct cld_msg_hdr *)&clnt->cl_u;
>>  
>> -	if (which != EV_READ)
>> -		goto out;
>> +	len = atomicio(read, clnt->cl_fd, hdr, hdr_len);
>>  
>> -	len = atomicio(read, clnt->cl_fd, cmsg, sizeof(*cmsg));
>>  	if (len <= 0) {
>>  		xlog(L_ERROR, "%s: pipe read failed: %m", __func__);
>> -		cld_pipe_open(clnt);
>> -		goto out;
>> +		goto fail_read;
>>  	}
> 
> We probably also want to fail if len != hdr_len.
>>  
>> -	if (cmsg->cm_vers > UPCALL_VERSION) {
>> +	switch (hdr->cm_vers) {
>> +	case 1:
>> +		left_len = sizeof(struct cld_msg) - hdr_len;
>> +		break;
>> +	case 2:
>> +		left_len = sizeof(struct cld_msg_v2) - hdr_len;
>> +		break;
>> +	default:
>>  		xlog(L_ERROR, "%s: unsupported upcall version: %hu",
>> -				__func__, cmsg->cm_vers);
>> -		cld_pipe_open(clnt);
>> -		goto out;
>> +			__func__, hdr->cm_vers);
>> +		goto fail_read;
>>  	}
>>  
>> -	switch(cmsg->cm_cmd) {
>> +	len = atomicio(read, clnt->cl_fd, hdr, left_len);
> 
> This is reading into the beginning of the message and overwriting the
> header.  In the original version of this patch you had hdr + 1 as the
> to read the data into.
> 
> -Scott
> 
>> +
>> +	if (len <= 0) {
>> +		xlog(L_ERROR, "%s: pipe read failed: %m", __func__);
>> +		goto fail_read;
>> +	}
>> +
>> +	return 0;
>> +
>> +fail_read:
>> +	cld_pipe_open(clnt);
>> +	return -1;
>> +}
>> +
>> +static void
>> +cldcb(int UNUSED(fd), short which, void *data)
>> +{
>> +	struct cld_client *clnt = data;
>> +	struct cld_msg_hdr *hdr = (struct cld_msg_hdr *)&clnt->cl_u;
>> +
>> +	if (which != EV_READ)
>> +		goto out;
>> +
>> +	if (cld_pipe_read_msg(clnt) < 0)
>> +		goto out;
>> +
>> +	switch (hdr->cm_cmd) {
>>  	case Cld_Create:
>>  		cld_create(clnt);
>>  		break;
>> @@ -765,7 +790,7 @@ cldcb(int UNUSED(fd), short which, void *data)
>>  		break;
>>  	default:
>>  		xlog(L_WARNING, "%s: command %u is not yet implemented",
>> -				__func__, cmsg->cm_cmd);
>> +				__func__, hdr->cm_cmd);
>>  		cld_not_implemented(clnt);
>>  	}
>>  out:
>> -- 
>> 2.33.0
>>
> 
>