Re: [PATCH 0/4] ipc: shm and msg fixes

[Linus Torvalds]

On Sun, Sep 22, 2013 at 11:42 PM, Davidlohr Bueso <davidlohr@xxxxxx> wrote:
>>
>> More importantly, it's wrong. You do the call_rcu() unconditionally,
>> but it might not be the last use! You need to do it with the same
>> logic ipc_rcu_putref(), namely at the dropping of the last reference.
>
> This is the way IPC has handled things for a long time, no? Security
> does not depend on the reference counter, as we unconditionally free
> security structs.

Yes, but that was ok back when the logic was idem-potent and you could
call it multiple times. Modulo races (I didn't check if we held a
lock).

You can't do "call_rcu()" more than once, because you'll corrupt the
rcu list if you do another call_rcu() while the first one is still
active (and that's a pretty big race window to hit).

That said, the old behavior was suspect for another reason too: having
the security blob go away from under a user sounds like it could cause
various other problems anyway, so I think the old code was at least
_prone_ to bugs even if it didn't have catastrophic behavior.

(In reality, I suspect the reference count is never elevated in
practice, so there is only one case that calls the security freeing
thing, so this may all be pretty much theoretical, but at least from a
logic standpoint the code clearly makes a big deal about the whole
refcount and "last user turns off the lights").

> What you're suggesting, is (i) freeing security will now depend on the
> refcount (wouldn't this cause cases where we actually never end up
> freeing?)

The security layer better not have any refcounted backpointers to the
shm, so I don't see why that would be a new issue.

>  and (ii) in the scenarios we actually need to free the
> security, delay it along with freeing the actual ipc_rcu stuff.

Well, that's the whole point. The security blob should have the same
lifetime as the ipc blob it is associated with.

Getting rid of the security blob before the thing it is supposed to
protect sounds like  a bug to me. In fact, it's the bug that this
whole thread has been about. No?

> If I understand correctly, then we'd have:
>
> void ipc_rcu_putref(void *ptr, void (*func)(struct rcu_head *head))
> {
>         struct ipc_rcu *p = ((struct ipc_rcu *)ptr) - 1;
>
>         if (!atomic_dec_and_test(&p->refcount))
>                 return;
>
>         call_rcu(&p->rcu, func);
> }

Exactly.

            Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/