Re: [PATCH -next] fs: Fix memory leaks in do_renameat2() error paths

From: ebiederm@xmission.com (Eric W. Biederman)
To: Jens Axboe <axboe@kernel.dk>
Cc: Al Viro <viro@zeniv.linux.org.uk>, Qian Cai <cai@redhat.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next] fs: Fix memory leaks in do_renameat2() error paths
Date: Mon, 02 Nov 2020 14:12:59 -0600	[thread overview]
Message-ID: <87k0v38qlw.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <f33a6b5e-ecc9-2bef-ab40-6bd8cc2030c2@kernel.dk> (Jens Axboe's message of "Mon, 2 Nov 2020 12:54:40 -0700")

Jens Axboe <axboe@kernel.dk> writes:

> On 11/2/20 12:27 PM, Eric W. Biederman wrote:
>> Jens Axboe <axboe@kernel.dk> writes:
>> 
>>> On 10/30/20 4:22 PM, Al Viro wrote:
>>>> On Fri, Oct 30, 2020 at 02:33:11PM -0600, Jens Axboe wrote:
>>>>> On 10/30/20 12:49 PM, Al Viro wrote:
>>>>>> On Fri, Oct 30, 2020 at 12:46:26PM -0600, Jens Axboe wrote:
>>>>>>
>>>>>>> See other reply, it's being posted soon, just haven't gotten there yet
>>>>>>> and it wasn't ready.
>>>>>>>
>>>>>>> It's a prep patch so we can call do_renameat2 and pass in a filename
>>>>>>> instead. The intent is not to have any functional changes in that prep
>>>>>>> patch. But once we can pass in filenames instead of user pointers, it's
>>>>>>> usable from io_uring.
>>>>>>
>>>>>> You do realize that pathname resolution is *NOT* offloadable to helper
>>>>>> threads, I hope...
>>>>>
>>>>> How so? If we have all the necessary context assigned, what's preventing
>>>>> it from working?
>>>>
>>>> Semantics of /proc/self/..., for starters (and things like /proc/mounts, etc.
>>>> *do* pass through that, /dev/stdin included)
>>>
>>> Don't we just need ->thread_pid for that to work?
>> 
>> No.  You need ->signal.
>> 
>> You need ->signal->pids[PIDTYPE_TGID].  It is only for /proc/thread-self
>> that ->thread_pid is needed.
>> 
>> Even more so than ->thread_pid, it is a kernel invariant that ->signal
>> does not change.
>
> I don't care about the pid itself, my suggestion was to assign ->thread_pid
> over the lookup operation to ensure that /proc/self/ worked the way that
> you'd expect.

I understand that.

However /proc/self/ refers to the current process not to the current
thread.  So ->thread_pid is not what you need to assign to make that
happen.  What the code looks at is: ->signal->pids[PIDTYPE_TGID].

It will definitely break invariants to assign to ->signal.

Currently only exchange_tids assigns ->thread_pid and it is nasty.  It
results in code that potentially results in infinite loops in
kernel/signal.c

To my knowledge nothing assigns ->signal->pids[PIDTYPE_TGID].  At best
it might work but I expect the it would completely confuse something in
the pid to task or pid to process mappings.  Which is to say even if it
does work it would be an extremely fragile solution.

Eric