Re: [PATCH] ptrace: Fix PTRACE_GETREGSET/PTRACE_SETREGSET in code documentation

[Pedro Alves <palves@redhat.com>]

On 05/05/14 05:10, Anshuman Khandual wrote:
> On 05/01/2014 07:43 PM, Pedro Alves wrote:
>> On 04/28/2014 12:00 PM, Anshuman Khandual wrote:
>>> The current documentation is bit misleading and does not explicitly
>>> specify that iov.len need to be initialized failing which kernel
>>> may just ignore the ptrace request and never read from/write into
>>> the user specified buffer. This patch fixes the documentation.
>>
>> Well, it kind of does, here:
>>
>> *      struct iovec iov = { buf, len};
> 
> :) Thats not explicit enough.
> 
>>
>>> @@ -43,8 +43,12 @@
>>>   *
>>>   *	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>>>   *
>>> - * On the successful completion, iov.len will be updated by the kernel,
>>> - * specifying how much the kernel has written/read to/from the user's iov.buf.
>>> + * A non-zero value upto the max size of data expected to be written/read by the
>>> + * kernel in response to any NT_XXX_TYPE request type must be assigned to iov.len
>>> + * before initiating the ptrace call. If iov.len is 0, then kernel will neither
>>> + * read from or write into the user buffer specified. On successful completion,
>>> + * iov.len will be updated by the kernel, specifying how much the kernel has
>>> + * written/read to/from the user's iov.buf.
>>
>> I really appreciate that you're trying to make this clearer, but I
>> find the new sentence very hard to read/reason.  :-/
>>
>> I suggest:
>>
>>  * This interface usage is as follows:
>> - *      struct iovec iov = { buf, len};
>> + *      struct iovec iov = { buf, len };
>>  *
>>  *      ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>>  *
>> - * On the successful completion, iov.len will be updated by the kernel,
>> - * specifying how much the kernel has written/read to/from the user's iov.buf.
>> + * On entry, iov describes the buffer's address and length.  The buffer's
>> + * length must be equal to or shorter than the size of the NT_XXX_TYPE regset.
>> + * On successful completion, iov.len is updated by the kernel, specifying how
>> + * much the kernel has written/read to/from the user's iov.buf.
>>
> 
> Yeah, sounds better. I may add "If the length is zero, the kernel will neither read
> from or write into the buffer"

Well, I think that much should be obvious.  What's not obvious is
whether that is considered success or error (what is the return code?)
I suspect and expect success return if the regset type is known, and
error otherwise.  So that could be used as a way to probe for support
for a given regset without using stack or heap space, if it ever matters.
The kernel never reads/writes beyond iov.len, so better say that, and
then it automatically gets the 0 case handled too, right?

>> I'm not sure I understood what you're saying correctly, though.  Specifically,
>> I don't know whether the buffer's length must really be shorter than the
>> size of the NT_XXX_TYPE regset.
> 
> No, it does not have to. From the code snippet below (ptrace_regset function)
> the buffer length has to be multiple of regset->size for the given NT_XXX_TYPE
> upto the max regset size for the user to see any valid data.

Ah, I guess one could call it a bug.  If the passed in
len is bigger than the whole register set size, then there seems
to be no point in validating whether the length is multiple of
a single register's size.  That unnecessarily prevents coming up
with a register set in the future that has registers of
different sizes...

But given that that's how things are today, I suppose we should
document it...

 The problem what I
> faced was when you use any iovec structure with the length parameter uninitialized,
> the kernel simply ignores and does not return anything.

Ah.  Well, saying "does not return anything" is quite confusing.  It does
return something -- -EINVAL.

> 
>         if (!regset || (kiov->iov_len % regset->size) != 0)
>                 return -EINVAL;

>  
>>
>>> The current documentation is bit misleading and does not explicitly
>>> specify that iov.len need to be initialized failing which kernel
>>> may just ignore the ptrace request and never read from/write into
>>> the user specified buffer.
>>
>> You're saying that if iov.len is larger than the NT_XXX_TYPE regset,
>> then the kernel returns _success_, but actually doesn't fill the
>> buffer?  That sounds like a bug to me.
> 
> No, I am not saying that. The kernel takes care of that situation by capping
> the length to regset size of the NT_XXX_TYPE.
> 
>  kiov->iov_len = min(kiov->iov_len,
>                             (__kernel_size_t) (regset->n * regset->size));
> 
> 

OK, then this is what I suggest instead:

   * This interface usage is as follows:
 - *      struct iovec iov = { buf, len};
 + *      struct iovec iov = { buf, len };
   *
   *      ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
   *
   * On the successful completion, iov.len will be updated by the kernel,
 - * specifying how much the kernel has written/read to/from the user's iov.buf.
 + * On entry, iov describes the buffer's address and length.  The buffer's
 + * length must be a multiple of the size of a single register in the register set.
 + * The kernel never reads or writes more than iov.len, and caps the buffer
 + * length to the register set's size.  In other words, the kernel reads or
 + * writes min(iov.len, regset size).
 + * On successful completion, iov.len is updated by the kernel, specifying how
 + * much the kernel has read from / written to the user's iov.buf.

> Shall I resend the patch with the your proposed changes and your "Signed-off-by" and
> moving myself as "Reported-by" ?

No idea of the actual policy to follow.  Feel free to do that if that's the
standard procedure.

-- 
Pedro Alves