Yafang Shao [off-list ref] writes:

On Sun, Jun 2, 2024 at 11:52 AM Eric W. Biederman [off-list ref] wrote:

quoted

Yafang Shao [off-list ref] writes:

quoted

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