Re: [PATCH] SUNRPC: fix refcounting problems with auth_gss messages.

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 1551 bytes --]

On Wed, Dec 07 2016, Olga Kornievskaia wrote:
>>>
>>> Agreed. This is a problem.
>>>
>>> Doesn't the problem still exist even with this patch because
>>> gss_add_msg() adds the msg onto the in_downcall() list? So gssd in
>>> __gss_fin_upcall() can find the 2nd upcall even before the 2nd msg is
>>> added to the pipe->pipe()?
>>
>> The use-after-free problem is solved I think.  It doesn't really make
>> any difference if the down-call arrives before or after
>> rpc_queue_upcall() is called.  The msg will still not be freed before it
>> is removed from both lists.
>>
>
> Sorry I don't see it.

Maybe we are looking at different code?

>
> Thread 1 adds an upcall and it's getting processed by gssd.
> Thread 2 executes gss_add_msg() which puts the message on the
> in_downcall list. Context switch (before the atomic_inc()!).

gss_add_msg(), as of 4.9-rc8, is
	spin_lock(&pipe->lock);
	old = __gss_find_upcall(pipe, gss_msg->uid, gss_msg->auth);
	if (old == NULL) {
		atomic_inc(&gss_msg->count);
		list_add(&gss_msg->list, &pipe->in_downcall);
	} else
		gss_msg = old;
	spin_unlock(&pipe->lock);

so the gss_msg is added to in_downcall *after* the atomic_inc(), and the
whole is protected by pipe->lock anyway so even if the atomic_inc() were
delayed by the CPU reordering things, there would be no risk of
gss_pipe_downcall() finding a gss_msg which didn't have the ->count
elevated.

NeilBrown

> Upcall comes back from the gssd, finds msg from Thread2 in_downcall
> list. gss_release_msg() will dec the counter to 0 and will remove the
> msg.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]