Yafang Shao <laoar.shao@xxxxxxxxx> writes: > On Sun, Jun 2, 2024 at 11:52 AM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote: >> >> Yafang Shao <laoar.shao@xxxxxxxxx> writes: >> >> > Quoted from Linus [0]: >> > >> > Since user space can randomly change their names anyway, using locking >> > was always wrong for readers (for writers it probably does make sense >> > to have some lock - although practically speaking nobody cares there >> > either, but at least for a writer some kind of race could have >> > long-term mixed results >> >> Ugh. >> Ick. >> >> This code is buggy. >> >> I won't argue that Linus is wrong, about removing the >> task_lock. >> >> Unfortunately strscpy_pad does not work properly with the >> task_lock removed, and buf_size larger that TASK_COMM_LEN. >> There is a race that will allow reading past the end >> of tsk->comm, if we read while tsk->common is being >> updated. > > It appears so. Thanks for pointing it out. Additionally, other code, > such as the BPF helper bpf_get_current_comm(), also uses strscpy_pad() > directly without the task_lock. It seems we should change that as > well. Which suggests that we could really use a helper that handles all of the tricky business of reading the tsk->comm lock-free. Eric
