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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/