[PATCH 7/7] sysvipc_find_ipc should increase position index

[PATCH 7/7] sysvipc_find_ipc should increase position index

[Vasily Averin]

if seq_file .next fuction does not change position index,
read after some lseek can generate unexpected output.

https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 ipc/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/util.c b/ipc/util.c
index 915eacb..7a3ab2e 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -764,13 +764,13 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
 			total++;
 	}
 
+	*new_pos = pos + 1;
 	if (total >= ids->in_use)
 		return NULL;
 
 	for (; pos < ipc_mni; pos++) {
 		ipc = idr_find(&ids->ipcs_idr, pos);
 		if (ipc != NULL) {
-			*new_pos = pos + 1;
 			rcu_read_lock();
 			ipc_lock_object(ipc);
 			return ipc;
-- 
1.8.3.1

Re: [PATCH 7/7] sysvipc_find_ipc should increase position index

[Waiman Long]

On 1/24/20 2:03 AM, Vasily Averin wrote:
> if seq_file .next fuction does not change position index,
> read after some lseek can generate unexpected output.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=206283
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  ipc/util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ipc/util.c b/ipc/util.c
> index 915eacb..7a3ab2e 100644
> --- a/ipc/util.c
> +++ b/ipc/util.c
> @@ -764,13 +764,13 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
>  			total++;
>  	}
>  
> +	*new_pos = pos + 1;
>  	if (total >= ids->in_use)
>  		return NULL;
>  
>  	for (; pos < ipc_mni; pos++) {
>  		ipc = idr_find(&ids->ipcs_idr, pos);
>  		if (ipc != NULL) {
> -			*new_pos = pos + 1;
>  			rcu_read_lock();
>  			ipc_lock_object(ipc);
>  			return ipc;

Acked-by: Waiman Long <longman@redhat.com>

Re: [PATCH 7/7] sysvipc_find_ipc should increase position index

[Vasily Averin]

Dear Andrew,
Could you please pick up this patch?

On 1/24/20 7:26 PM, Waiman Long wrote:
> On 1/24/20 2:03 AM, Vasily Averin wrote:
>> if seq_file .next fuction does not change position index,
>> read after some lseek can generate unexpected output.
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=206283
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
>> ---
>>  ipc/util.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ipc/util.c b/ipc/util.c
>> index 915eacb..7a3ab2e 100644
>> --- a/ipc/util.c
>> +++ b/ipc/util.c
>> @@ -764,13 +764,13 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
>>  			total++;
>>  	}
>>  
>> +	*new_pos = pos + 1;
>>  	if (total >= ids->in_use)
>>  		return NULL;
>>  
>>  	for (; pos < ipc_mni; pos++) {
>>  		ipc = idr_find(&ids->ipcs_idr, pos);
>>  		if (ipc != NULL) {
>> -			*new_pos = pos + 1;
>>  			rcu_read_lock();
>>  			ipc_lock_object(ipc);
>>  			return ipc;
> 
> Acked-by: Waiman Long <longman@redhat.com>
> 

Re: [PATCH 7/7] sysvipc_find_ipc should increase position index

[Andreas Schwab]

I think this is causing this bug (seen on 5.6.8):

# ipcs -q

------ Message Queues --------
key        msqid      owner      perms      used-bytes   messages    

# ipcmk -Q
Message queue id: 0
# ipcs -q

------ Message Queues --------
key        msqid      owner      perms      used-bytes   messages    
0x82db8127 0          root       644        0            0           

# ipcmk -Q
Message queue id: 1
# ipcs -q

------ Message Queues --------
key        msqid      owner      perms      used-bytes   messages    
0x82db8127 0          root       644        0            0           
0x76d1fb2a 1          root       644        0            0           

# ipcrm -q 0
# ipcs -q

------ Message Queues --------
key        msqid      owner      perms      used-bytes   messages    
0x76d1fb2a 1          root       644        0            0           
0x76d1fb2a 1          root       644        0            0           

# ipcmk -Q
Message queue id: 2
# ipcrm -q 2
# ipcs -q

------ Message Queues --------
key        msqid      owner      perms      used-bytes   messages    
0x76d1fb2a 1          root       644        0            0           
0x76d1fb2a 1          root       644        0            0           

# ipcmk -Q
Message queue id: 3
# ipcrm -q 1
# ipcs -q

------ Message Queues --------
key        msqid      owner      perms      used-bytes   messages    
0x7c982867 3          root       644        0            0           
0x7c982867 3          root       644        0            0           
0x7c982867 3          root       644        0            0           
0x7c982867 3          root       644        0            0           


As you can see, whenever an IPC item with a low id is deleted, the items
with higher ids are duplicated, as if filling a hole.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 7/7] sysvipc_find_ipc should increase position index

[Vasily Averin]

On 5/5/20 7:13 PM, Andreas Schwab wrote:
> I think this is causing this bug (seen on 5.6.8):

thank you for reporting,
yes, you are right, it's my fault
patch incorrectly updated *new_pos in case of found entry

> # ipcs -q
> 
> ------ Message Queues --------
> key        msqid      owner      perms      used-bytes   messages    
> 0x7c982867 3          root       644        0            0           
> 0x7c982867 3          root       644        0            0           
> 0x7c982867 3          root       644        0            0           
> 0x7c982867 3          root       644        0            0           

Thank you,
	Vasily Averin

[PATCH] ipc/util.c: sysvipc_find_ipc() incorrectly updates position index

[Vasily Averin]

new_pos should jump through hole of unused ids,
pos can be updated inside "for" cycle.

Cc: stable@vger.kernel.org
Fixes: 89163f93c6f9 ("ipc/util.c: sysvipc_find_ipc() should increase position index")
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 ipc/util.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ipc/util.c b/ipc/util.c
index 7acccfd..cfa0045 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -764,21 +764,21 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
 			total++;
 	}
 
-	*new_pos = pos + 1;
+	ipc = NULL;
 	if (total >= ids->in_use)
-		return NULL;
+		goto out;
 
 	for (; pos < ipc_mni; pos++) {
 		ipc = idr_find(&ids->ipcs_idr, pos);
 		if (ipc != NULL) {
 			rcu_read_lock();
 			ipc_lock_object(ipc);
-			return ipc;
+			break;
 		}
 	}
-
-	/* Out of range - return NULL to terminate iteration */
-	return NULL;
+out:
+	*new_pos = pos + 1;
+	return ipc;
 }
 
 static void *sysvipc_proc_next(struct seq_file *s, void *it, loff_t *pos)
-- 
1.8.3.1

Re: [PATCH] ipc/util.c: sysvipc_find_ipc() incorrectly updates position index

[Waiman Long]

On 5/6/20 2:25 AM, Vasily Averin wrote:
> new_pos should jump through hole of unused ids,
> pos can be updated inside "for" cycle.
>
> Cc: stable@vger.kernel.org
> Fixes: 89163f93c6f9 ("ipc/util.c: sysvipc_find_ipc() should increase position index")
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>   ipc/util.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/ipc/util.c b/ipc/util.c
> index 7acccfd..cfa0045 100644
> --- a/ipc/util.c
> +++ b/ipc/util.c
> @@ -764,21 +764,21 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
>   			total++;
>   	}
>   
> -	*new_pos = pos + 1;
> +	ipc = NULL;
>   	if (total >= ids->in_use)
> -		return NULL;
> +		goto out;
>   
>   	for (; pos < ipc_mni; pos++) {
>   		ipc = idr_find(&ids->ipcs_idr, pos);
>   		if (ipc != NULL) {
>   			rcu_read_lock();
>   			ipc_lock_object(ipc);
> -			return ipc;
> +			break;
>   		}
>   	}
> -
> -	/* Out of range - return NULL to terminate iteration */
> -	return NULL;
> +out:
> +	*new_pos = pos + 1;
> +	return ipc;
>   }
>   
>   static void *sysvipc_proc_next(struct seq_file *s, void *it, loff_t *pos)

Acked-by: Waiman Long <longman@redhat.com>

Re: [PATCH] ipc/util.c: sysvipc_find_ipc() incorrectly updates position index

[Vasily Averin]

Dear Andrew,
could you please handle it,
it fixes broken ipcs in last mainline and stable kernels,
and on all its derivatives.

Thank you,
	Vasily Averin

On 5/6/20 6:59 PM, Waiman Long wrote:
> On 5/6/20 2:25 AM, Vasily Averin wrote:
>> new_pos should jump through hole of unused ids,
>> pos can be updated inside "for" cycle.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 89163f93c6f9 ("ipc/util.c: sysvipc_find_ipc() should increase position index")
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
>> ---
>>   ipc/util.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/ipc/util.c b/ipc/util.c
>> index 7acccfd..cfa0045 100644
>> --- a/ipc/util.c
>> +++ b/ipc/util.c
>> @@ -764,21 +764,21 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
>>               total++;
>>       }
>>   -    *new_pos = pos + 1;
>> +    ipc = NULL;
>>       if (total >= ids->in_use)
>> -        return NULL;
>> +        goto out;
>>         for (; pos < ipc_mni; pos++) {
>>           ipc = idr_find(&ids->ipcs_idr, pos);
>>           if (ipc != NULL) {
>>               rcu_read_lock();
>>               ipc_lock_object(ipc);
>> -            return ipc;
>> +            break;
>>           }
>>       }
>> -
>> -    /* Out of range - return NULL to terminate iteration */
>> -    return NULL;
>> +out:
>> +    *new_pos = pos + 1;
>> +    return ipc;
>>   }
>>     static void *sysvipc_proc_next(struct seq_file *s, void *it, loff_t *pos)
> 
> Acked-by: Waiman Long <longman@redhat.com>
> 

Re: [PATCH] ipc/util.c: sysvipc_find_ipc() incorrectly updates position index

[Andrew Morton]

On Wed, 6 May 2020 09:25:54 +0300 Vasily Averin <vvs@virtuozzo.com> wrote:

> new_pos should jump through hole of unused ids,
> pos can be updated inside "for" cycle.
> 
> Cc: stable@vger.kernel.org
> Fixes: 89163f93c6f9 ("ipc/util.c: sysvipc_find_ipc() should increase position index")

This:

> --- a/ipc/util.c
> +++ b/ipc/util.c
> @@ -764,21 +764,21 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
>  			total++;
>  	}
>  
> -	*new_pos = pos + 1;
> +	ipc = NULL;
>  	if (total >= ids->in_use)
> -		return NULL;
> +		goto out;
>  
>  	for (; pos < ipc_mni; pos++) {
>  		ipc = idr_find(&ids->ipcs_idr, pos);
>  		if (ipc != NULL) {
>  			rcu_read_lock();
>  			ipc_lock_object(ipc);
> -			return ipc;
> +			break;
>  		}
>  	}
> -
> -	/* Out of range - return NULL to terminate iteration */
> -	return NULL;
> +out:
> +	*new_pos = pos + 1;
> +	return ipc;
>  }
>  
>  static void *sysvipc_proc_next(struct seq_file *s, void *it, loff_t *pos)

Messes up Matthew's "ipc: convert ipcs_idr to XArray"
(http://lkml.kernel.org/r/20200326151418.27545-1-willy@infradead.org).

Here's how I resolved things.  Please check?

static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
					      loff_t *new_pos)
{
	unsigned long index = pos;
	struct kern_ipc_perm *ipc;

	rcu_read_lock();
	ipc = xa_find(&ids->ipcs, &index, ULONG_MAX, XA_PRESENT);
	if (ipc)
		ipc_lock_object(ipc);
	else
		rcu_read_unlock();
	*new_pos = pos + 1;
	return ipc;
}

Re: [PATCH] ipc/util.c: sysvipc_find_ipc() incorrectly updates position index

[Matthew Wilcox]

On Thu, May 07, 2020 at 05:02:42PM -0700, Andrew Morton wrote:
> Here's how I resolved things.  Please check?
> 
> static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
> 					      loff_t *new_pos)
> {
> 	unsigned long index = pos;
> 	struct kern_ipc_perm *ipc;
> 
> 	rcu_read_lock();
> 	ipc = xa_find(&ids->ipcs, &index, ULONG_MAX, XA_PRESENT);
> 	if (ipc)
> 		ipc_lock_object(ipc);
> 	else
> 		rcu_read_unlock();
> 	*new_pos = pos + 1;
> 	return ipc;
> }

Surely that should be '*new_pos = index + 1'?  Or did I misunderstand
the reasoning behind the other patch?

Re: [PATCH] ipc/util.c: sysvipc_find_ipc() incorrectly updates position index

[Vasily Averin]

On 5/8/20 6:36 AM, Matthew Wilcox wrote:
> On Thu, May 07, 2020 at 05:02:42PM -0700, Andrew Morton wrote:
>> Here's how I resolved things.  Please check?
>>
>> static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
>> 					      loff_t *new_pos)
>> {
>> 	unsigned long index = pos;
>> 	struct kern_ipc_perm *ipc;
>>
>> 	rcu_read_lock();
>> 	ipc = xa_find(&ids->ipcs, &index, ULONG_MAX, XA_PRESENT);
>> 	if (ipc)
>> 		ipc_lock_object(ipc);
>> 	else
>> 		rcu_read_unlock();
>> 	*new_pos = pos + 1;
>> 	return ipc;
>> }
> 
> Surely that should be '*new_pos = index + 1'?  Or did I misunderstand
> the reasoning behind the other patch?

I'm not sure however it looks like xa_find() can return index < pos
xa_find in our case will call xas_find_marked() that have following description

 * If no marked entry is found and the array is smaller than @max, @xas is
 * set to the bounds state and xas->xa_index is set to the smallest index
 * not yet in the array.  This allows @xas to be immediately passed to
 * xas_store().

Matthew, could you please clarify this question?

Thank you,
	Vasily Averin

Re: [PATCH] ipc/util.c: sysvipc_find_ipc() incorrectly updates position index

[Vasily Averin]

On 5/8/20 9:07 AM, Vasily Averin wrote:
> On 5/8/20 6:36 AM, Matthew Wilcox wrote:
>> On Thu, May 07, 2020 at 05:02:42PM -0700, Andrew Morton wrote:
>>> Here's how I resolved things.  Please check?
>>>
>>> static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
>>> 					      loff_t *new_pos)
>>> {
>>> 	unsigned long index = pos;
>>> 	struct kern_ipc_perm *ipc;
>>>
>>> 	rcu_read_lock();
>>> 	ipc = xa_find(&ids->ipcs, &index, ULONG_MAX, XA_PRESENT);
>>> 	if (ipc)
>>> 		ipc_lock_object(ipc);
>>> 	else
>>> 		rcu_read_unlock();
>>> 	*new_pos = pos + 1;
>>> 	return ipc;
>>> }
>>
>> Surely that should be '*new_pos = index + 1'?  Or did I misunderstand
>> the reasoning behind the other patch?
> 
> I'm not sure however it looks like xa_find() can return index < pos
it seems, I was wrong here.
So I'm agree with Matthew, '*new_pos = index + 1' should be used.

Thank you,
	Vasily Averin

Re: [PATCH] ipc/util.c: sysvipc_find_ipc() incorrectly updates position index

[Jiri Slaby]

On 08. 05. 20, 12:01, Vasily Averin wrote:
> On 5/8/20 9:07 AM, Vasily Averin wrote:
>> On 5/8/20 6:36 AM, Matthew Wilcox wrote:
>>> On Thu, May 07, 2020 at 05:02:42PM -0700, Andrew Morton wrote:
>>>> Here's how I resolved things.  Please check?
>>>>
>>>> static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
>>>> 					      loff_t *new_pos)
>>>> {
>>>> 	unsigned long index = pos;
>>>> 	struct kern_ipc_perm *ipc;
>>>>
>>>> 	rcu_read_lock();
>>>> 	ipc = xa_find(&ids->ipcs, &index, ULONG_MAX, XA_PRESENT);
>>>> 	if (ipc)
>>>> 		ipc_lock_object(ipc);
>>>> 	else
>>>> 		rcu_read_unlock();
>>>> 	*new_pos = pos + 1;
>>>> 	return ipc;
>>>> }
>>>
>>> Surely that should be '*new_pos = index + 1'?  Or did I misunderstand
>>> the reasoning behind the other patch?
>>
>> I'm not sure however it looks like xa_find() can return index < pos
> it seems, I was wrong here.
> So I'm agree with Matthew, '*new_pos = index + 1' should be used.

Any progress on this? 5.7-rc*, 5.4.40, and 5.6.12 are still affected.

Wouldn't it be better to rebase (apply the originally submitted patch)
before the XA rewrite and push that one to Linus?

thanks,
-- 
js
suse labs

Re: [PATCH] ipc/util.c: sysvipc_find_ipc() incorrectly updates position index

[Vasily Averin]

On 5/12/20 12:21 PM, Jiri Slaby wrote:
> On 08. 05. 20, 12:01, Vasily Averin wrote:
>> On 5/8/20 9:07 AM, Vasily Averin wrote:
>>> On 5/8/20 6:36 AM, Matthew Wilcox wrote:
>>>> On Thu, May 07, 2020 at 05:02:42PM -0700, Andrew Morton wrote:
>>>>> Here's how I resolved things.  Please check?
>>>>>
>>>>> static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
>>>>> 					      loff_t *new_pos)
>>>>> {
>>>>> 	unsigned long index = pos;
>>>>> 	struct kern_ipc_perm *ipc;
>>>>>
>>>>> 	rcu_read_lock();
>>>>> 	ipc = xa_find(&ids->ipcs, &index, ULONG_MAX, XA_PRESENT);
>>>>> 	if (ipc)
>>>>> 		ipc_lock_object(ipc);
>>>>> 	else
>>>>> 		rcu_read_unlock();
>>>>> 	*new_pos = pos + 1;
>>>>> 	return ipc;
>>>>> }
>>>>
>>>> Surely that should be '*new_pos = index + 1'?  Or did I misunderstand
>>>> the reasoning behind the other patch?
>>>
>>> I'm not sure however it looks like xa_find() can return index < pos
>> it seems, I was wrong here.
>> So I'm agree with Matthew, '*new_pos = index + 1' should be used.
> 
> Any progress on this? 5.7-rc*, 5.4.40, and 5.6.12 are still affected.

Andrew included fix to -mm tree and I hope he'll push it to mainline/stable soon.
https://ozlabs.org/~akpm/mmots/broken-out/ipc-utilc-sysvipc_find_ipc-incorrectly-updates-position-index.patch

> Wouldn't it be better to rebase (apply the originally submitted patch)
> before the XA rewrite and push that one to Linus?
I'm expecting thins too. 

Thank you,
	Vasily Averin