Re: [PATCH 0/2] shm: omit forced shm destroy if task IPC namespace was changed

[Manfred Spraul]

Hi Eric,

I'd like to restart the discussion, the issue should be fixed.

On 7/12/21 9:18 PM, Eric W. Biederman wrote:
> Given that this is a bug I think c) is the safest option.
>
> A couple of suggestions.
> 1) We can replace the test "shm_creator != NULL" with
>     "list_empty(&shp->shm_clist)"

Yes, good idea. list_del() already contains WRITE_ONCE() and 
list_empty() contains READ_ONCE(), i.e. we get clear memory ordering rules.


>   and remove shm_creator.

I do not see how we can remove shm_creator:

- thread A: creates new segment, doesn't map it.

- thread B: shmctl(,IPC_RMID,).

thread B must now locate the lock that protects ->shm_clist. And if the 
lock is per-thread, then I do not see how we can avoid having a pointer 
in shp to the lock.


>     Along with replacing "shm_creator = NULL" with
>     "list_del_init(&shp->shm_clist)".
Correct, list_del_init() is better than shm_create = NULL.
> 2) We can update shmat to do "list_del_init(&shp->shm_clist)"
>     upon shmat.

That would be a (tiny) user space visible change:

echo 0 > /proc/sys/kernel/shm_rmid_forced
shmget()
shmat()
shmdt()
echo 1 > /proc/sys/kernel/shm_rmid_forced
exit()

Right now: segment is destroyed

After your proposal: Segment is not destroyed.

I don't think that we should mix that with the bugfix.


>    The last unmap will still shm_destroy the
>     shm segment as ns->shm_rmid_forced is set.
But what if shm_rmid_forced is modified?
>     For a multi-threaded process I think this will nicely clean up
>     the clist, and make it clear that the clist only cares about
>     those segments that have been created but never attached.

> 3) Put a non-reference counted struct ipc_namespace in struct
>     shmid_kernel, and use it to remove the namespace parameter
>     from shm_destroy.
>
> I think that is enough to fix this bug with no changes in semantics,
> no additional memory consumed, and an implementation that is easier
> to read and perhaps a little faster.

I do not see how this solves the list corruption:

A thread creates 2 shm segments, then switches the namespace and creates 
another 2 segment.

- corruption 1: in each of the namespaces, one thread calls 
shmctl(,IPC_RMID,) -> both will operate in parallel on ->shm_clist.

- corruption 2: exit_shm() in parallel to one thread

- corruption 3: one shmctl(,IPC_RMID,) in parallel to a shmget().

i.e.: we can have list_add() and multiple list_del() in parallel.

I don't see how this should work without a lock.

With regards to memory usage: I would propose to use task_lock(), that 
lock exists already.


--

    Manfred