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

Manfred Spraul
2021-09-23T16:36:23Z

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