Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> writes: > On Sun, Jun 2, 2024 at 10:53 AM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote: >> >> If you are performing lockless reads and depending upon a '\0' >> terminator without limiting yourself to the size of the buffer >> there needs to be a big fat comment as to how in the world >> you are guaranteed that a '\0' inside the buffer will always >> be found. > > I think Yafang can certainly add such a comment next to > __[gs]et_task_comm. > > I prefer to avoid open coding memcpy + mmemset when strscpy_pad works. Looking through the code in set_task_comm strscpy_pad only works when both the source and designation are aligned. Otherwise it performs a byte a time copy, and is most definitely susceptible to the race I observed. Further I looked a couple of the uses of set_task_com, in fs/proc/base.c, kernel/kthread.c, and kernel/sys.c. Nowhere do I see a guarantee that the source buffer is word aligned or even something that would reasonably cause a compiler to place the buffer that is being passed to set_task_comm to be word aligned. As far as I can tell it is completely up to the compiler if it will cause strscpy_pad to honor the word at a time guarantee needed to make strscpy_pad safe for reading the information. This is not to say we can't make it safe. The easiest would be to create an aligned temporary buffer in set_task_comm, and preserve the existing interface. Alternatively a type that has the appropriate size and alignment could be used as input to set_task_comm and it could be caller's responsibility to use it. While we can definitely make reading task->comm happen without taking the lock. Doing so without updating set_task_comm to provide the guarantees needed to make it safe, looks like a case of play silly games, win silly prizes. Eric
