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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
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, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
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
new_pos should jump through hole of unused ids,
pos can be updated inside "for" cycle.
Cc: [email protected]
Fixes: 89163f93c6f9 ("ipc/util.c: sysvipc_find_ipc() should increase position index")
Signed-off-by: Vasily Averin <[email protected]>
---
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
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: [email protected]
> Fixes: 89163f93c6f9 ("ipc/util.c: sysvipc_find_ipc() should increase position index")
> Signed-off-by: Vasily Averin <[email protected]>
> ---
> 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 <[email protected]>
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: [email protected]
>> Fixes: 89163f93c6f9 ("ipc/util.c: sysvipc_find_ipc() should increase position index")
>> Signed-off-by: Vasily Averin <[email protected]>
>> ---
>> 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 <[email protected]>
>
On Wed, 6 May 2020 09:25:54 +0300 Vasily Averin <[email protected]> wrote:
> new_pos should jump through hole of unused ids,
> pos can be updated inside "for" cycle.
>
> Cc: [email protected]
> 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/[email protected]).
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;
}
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?
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
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
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
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