Re: [PATCH 4/5] ipc/msg: Lockless security checks for msgsnd

Re: [PATCH 4/5] ipc/msg: Lockless security checks for msgsnd

[Manfred Spraul]

Hi Davidlohr,

> Just as with msgrcv (along with the rest of sysvipc since a few years
>     ago), perform the security checks without holding the ipc object lock.
Thinking about it: isn't this wrong?

CPU1:
* msgrcv()
* ipcperms()
<sleep>

CPU2:
* msgctl(), change permissions
** msgctl() returns, new permissions should now be in effect
* msgsnd(), send secret message
** msgsnd() returns, new message stored.

CPU1: resumes, receives secret message

Obviously, we could argue that the msgrcv() was already ongoing and 
therefore the old permissions still apply - but then we don't need to 
recheck after sleeping at all.


>     This also reduces the hogging of the lock for the entire duration of a
>     sender, as we drop the lock upon every iteration -- and this is 
> exactly
>     why we also check for racing with RMID in the first place.

Which hogging do you mean? The lock is dopped uppon every iteration, the 
schedule() is in the middle.
Which your patch, the lock are now dropped twice:
> -
>   	for (;;) {
>   		struct msg_sender s;
>   
>   		err = -EACCES;
>   		if (ipcperms(ns, &msq->q_perm, S_IWUGO))
> -			goto out_unlock0;
> +			goto out_unlock1;
> +
> +		ipc_lock_object(&msq->q_perm);
>   
>   		/* raced with RMID? */
>   		if (!ipc_valid_object(&msq->q_perm)) {
> @@ -681,6 +681,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
>   			goto out_unlock0;
>   		}
>   
> +		ipc_unlock_object(&msq->q_perm);
>   	}
>
>
This means the lock is dropped, just for ipcperms().
This doubles the lock acquire/release cycles.

--
     Manfred

Re: [PATCH 4/5] ipc/msg: Lockless security checks for msgsnd

[Davidlohr Bueso]

On Sun, 18 Sep 2016, Manfred Spraul wrote:

>>Just as with msgrcv (along with the rest of sysvipc since a few years
>>    ago), perform the security checks without holding the ipc object lock.
>Thinking about it: isn't this wrong?
>
>CPU1:
>* msgrcv()
>* ipcperms()
><sleep>
>
>CPU2:
>* msgctl(), change permissions
>** msgctl() returns, new permissions should now be in effect
>* msgsnd(), send secret message
>** msgsnd() returns, new message stored.
>
>CPU1: resumes, receives secret message

Hmm, would this not apply to everything IPC_SET, we do lockless ipcperms()
all over the place.

>Obviously, we could argue that the msgrcv() was already ongoing and 
>therefore the old permissions still apply - but then we don't need to 
>recheck after sleeping at all.

There is that, and furthermore we make no such guarantees under concurrency.
Another way of looking at it could perhaps be IPC_SET returning EPERM if
there's an unserviced msgrcv -- but I'm not suggesting doing this btw ;)

>
>>    This also reduces the hogging of the lock for the entire duration of a
>>    sender, as we drop the lock upon every iteration -- and this is 
>>exactly
>>    why we also check for racing with RMID in the first place.
>
>Which hogging do you mean? The lock is dopped uppon every iteration, 
>the schedule() is in the middle.
>Which your patch, the lock are now dropped twice:
>>-
>>  	for (;;) {
>>  		struct msg_sender s;
>>  		err = -EACCES;
>>  		if (ipcperms(ns, &msq->q_perm, S_IWUGO))
>>-			goto out_unlock0;
>>+			goto out_unlock1;
>>+
>>+		ipc_lock_object(&msq->q_perm);
>>  		/* raced with RMID? */
>>  		if (!ipc_valid_object(&msq->q_perm)) {
>>@@ -681,6 +681,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
>>  			goto out_unlock0;
>>  		}
>>+		ipc_unlock_object(&msq->q_perm);
>>  	}
>>
>>
>This means the lock is dropped, just for ipcperms().
>This doubles the lock acquire/release cycles.

The effectiveness all depends on the workload and degree of contention. But
I have no problem dropping this patch either, although this is standard for
all things ipc.

Thanks,
Davidlohr

Re: [PATCH 4/5] ipc/msg: Lockless security checks for msgsnd

[Manfred Spraul]

On 09/22/2016 12:21 AM, Davidlohr Bueso wrote:
> On Sun, 18 Sep 2016, Manfred Spraul wrote:
>
>>> Just as with msgrcv (along with the rest of sysvipc since a few years
>>>    ago), perform the security checks without holding the ipc object 
>>> lock.
>> Thinking about it: isn't this wrong?
>>
>> CPU1:
>> * msgrcv()
>> * ipcperms()
>> <sleep>
>>
>> CPU2:
>> * msgctl(), change permissions
>> ** msgctl() returns, new permissions should now be in effect
>> * msgsnd(), send secret message
>> ** msgsnd() returns, new message stored.
>>
>> CPU1: resumes, receives secret message
>
> Hmm, would this not apply to everything IPC_SET, we do lockless 
> ipcperms()
> all over the place.
>
>> Obviously, we could argue that the msgrcv() was already ongoing and 
>> therefore the old permissions still apply - but then we don't need to 
>> recheck after sleeping at all.
>
> There is that, and furthermore we make no such guarantees under 
> concurrency.
> Another way of looking at it could perhaps be IPC_SET returning EPERM if
> there's an unserviced msgrcv -- but I'm not suggesting doing this btw ;)
I looked at it by comparing how we handle ipcperms and 
security_msg_queue_xx():
The security_msg_queue_xx are called under the lock, or actually the 
msgrcv even looks at the message that is transferred.

For ipc/sem.c and ipc/shm.c, both operations are called outside the lock.

So, my proposal would be that ipcperms and security_xx should show the 
same behavior regarding concurrent ops.
And this would mean that we should not move ipcperms() in msgrcv outside 
of the lock, i.e. drop the patch.

But if you have a better rational, feel free. But please document it in 
the changelog.

--
     Manfred

[PATCH 0/5] ipc/msg: Sender/receiver optimizations

[Davidlohr Bueso]

Hi,

I'm resending Sebastian's sysv msg queue use of wake_qs but updated
to the last observations I need wrt the need of explicit barriers
after removing the whole receiver busy-looping. After some irc exchange
it seems we're both on the same page, and things now look like he had
them earlier, in v2. This is all patch 1.

The rest of the patches are changes I noticed while reviewing patch 1,
which are mainly sender-side rework/optimizations. Details are in each
changelog.

The changes have survived ltp (which has some nasty corner cases for msgsnd
changes), as well as pmsg-shared benchmark.

Applies on Linus's latest - please consider for v4.9.

Thanks!

  ipc/msg: Implement lockless pipelined wakeups
  ipc/msg: Batch queue sender wakeups
  ipc/msg: Make ss_wakeup() kill arg boolean
  ipc/msg: Lockless security checks for msgsnd
  ipc/msg: Avoid waking sender upon full queue

 ipc/msg.c | 210 ++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 101 insertions(+), 109 deletions(-)

-- 
2.6.6

[PATCH 4/5] ipc/msg: Lockless security checks for msgsnd

[Davidlohr Bueso]

Just as with msgrcv (along with the rest of sysvipc since a few
years ago), perform the security checks without holding the ipc
object lock. This also reduces the hogging of the lock for the
entire duration of a sender, as we drop the lock upon every
iteration -- and this is exactly why we also check for racing
with RMID in the first place.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 ipc/msg.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 5181259e2ff0..fe793304dddb 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -623,14 +623,14 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
 		goto out_unlock1;
 	}
 
-	ipc_lock_object(&msq->q_perm);
-
 	for (;;) {
 		struct msg_sender s;
 
 		err = -EACCES;
 		if (ipcperms(ns, &msq->q_perm, S_IWUGO))
-			goto out_unlock0;
+			goto out_unlock1;
+
+		ipc_lock_object(&msq->q_perm);
 
 		/* raced with RMID? */
 		if (!ipc_valid_object(&msq->q_perm)) {
@@ -681,6 +681,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
 			goto out_unlock0;
 		}
 
+		ipc_unlock_object(&msq->q_perm);
 	}
 	msq->q_lspid = task_tgid_vnr(current);
 	msq->q_stime = get_seconds();
-- 
2.6.6