[PATCH 1/1] cio_ignore_proc_seq_next should increase position index

[PATCH 1/1] cio_ignore_proc_seq_next 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>
---
 drivers/s390/cio/blacklist.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/blacklist.c b/drivers/s390/cio/blacklist.c
index 2a3f874..9cebff8 100644
--- a/drivers/s390/cio/blacklist.c
+++ b/drivers/s390/cio/blacklist.c
@@ -303,8 +303,10 @@ struct ccwdev_iter {
 cio_ignore_proc_seq_next(struct seq_file *s, void *it, loff_t *offset)
 {
 	struct ccwdev_iter *iter;
+	loff_t p = *offset;
 
-	if (*offset >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
+	(*offset)++;
+	if (p >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
 		return NULL;
 	iter = it;
 	if (iter->devno == __MAX_SUBCHANNEL) {
@@ -314,7 +316,6 @@ struct ccwdev_iter {
 			return NULL;
 	} else
 		iter->devno++;
-	(*offset)++;
 	return iter;
 }
 
-- 
1.8.3.1

Re: [PATCH 1/1] cio_ignore_proc_seq_next should increase position index

[Cornelia Huck]

On Fri, 24 Jan 2020 08:48:55 +0300
Vasily Averin <vvs@virtuozzo.com> wrote:

$SUBJECT: "s390/cio: ..." ?

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

Fixes: 678a395b356a ("[PATCH] s390: convert /proc/cio_ignore")

> https://bugzilla.kernel.org/show_bug.cgi?id=206283
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  drivers/s390/cio/blacklist.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/blacklist.c b/drivers/s390/cio/blacklist.c
> index 2a3f874..9cebff8 100644
> --- a/drivers/s390/cio/blacklist.c
> +++ b/drivers/s390/cio/blacklist.c
> @@ -303,8 +303,10 @@ struct ccwdev_iter {
>  cio_ignore_proc_seq_next(struct seq_file *s, void *it, loff_t *offset)
>  {
>  	struct ccwdev_iter *iter;
> +	loff_t p = *offset;

I was asking myself if we could avoid the new variable, but anything I
could think of looked worse.

>  
> -	if (*offset >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
> +	(*offset)++;
> +	if (p >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
>  		return NULL;
>  	iter = it;
>  	if (iter->devno == __MAX_SUBCHANNEL) {
> @@ -314,7 +316,6 @@ struct ccwdev_iter {
>  			return NULL;
>  	} else
>  		iter->devno++;
> -	(*offset)++;
>  	return iter;
>  }
>  

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 1/1] cio_ignore_proc_seq_next should increase position index

[Christian Borntraeger]

On 24.01.20 06:48, 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>
> ---
>  drivers/s390/cio/blacklist.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/blacklist.c b/drivers/s390/cio/blacklist.c
> index 2a3f874..9cebff8 100644
> --- a/drivers/s390/cio/blacklist.c
> +++ b/drivers/s390/cio/blacklist.c
> @@ -303,8 +303,10 @@ struct ccwdev_iter {
>  cio_ignore_proc_seq_next(struct seq_file *s, void *it, loff_t *offset)
>  {
>  	struct ccwdev_iter *iter;
> +	loff_t p = *offset;
>  
> -	if (*offset >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
> +	(*offset)++;
> +	if (p >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
>  		return NULL;
>  	iter = it;
>  	if (iter->devno == __MAX_SUBCHANNEL) {
> @@ -314,7 +316,6 @@ struct ccwdev_iter {
>  			return NULL;
>  	} else
>  		iter->devno++;
> -	(*offset)++;
>  	return iter;
>  }
>  
> 

I guess this fixes one aspect:
"dd: /proc/cio_ignore: cannot skip to specified offset"
is now gone. So I am tempted to apply this. 

but this code is still fishy:

$ cat /proc/cio_ignore 
0.0.fe00-0.0.fefe
0.0.ff00-0.0.ffff
$ dd if=/proc/cio_ignore status=none
0.0.fe00-0.0.fefe
0.0.ff00-0.0.ffff
$ dd if=/proc/cio_ignore status=none bs=10
0.0.fe00-0.0.fefe
0.0.ff00-0.0.ff01-0.0.ff02-0.0.ff03-0.0.ff04-0.0.ff05-0.0.ff06-0.0.ff07-0.0.ff08-0.0.ffff
$ dd if=/proc/cio_ignore status=none bs=10 skip=1
.0.fefe
0.0.ff00-0.0.ff01-0.0.ff02-0.0.ff03-0.0.ff04-0.0.ff05-0.0.ff06-0.0.ff07-0.0.ff08-0.0.ffff


Peter, any opinions on this?

Re: [PATCH 1/1] cio_ignore_proc_seq_next should increase position index

[Cornelia Huck]

On Fri, 7 Feb 2020 14:13:05 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 24.01.20 06:48, 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>
> > ---
> >  drivers/s390/cio/blacklist.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/blacklist.c b/drivers/s390/cio/blacklist.c
> > index 2a3f874..9cebff8 100644
> > --- a/drivers/s390/cio/blacklist.c
> > +++ b/drivers/s390/cio/blacklist.c
> > @@ -303,8 +303,10 @@ struct ccwdev_iter {
> >  cio_ignore_proc_seq_next(struct seq_file *s, void *it, loff_t *offset)
> >  {
> >  	struct ccwdev_iter *iter;
> > +	loff_t p = *offset;
> >  
> > -	if (*offset >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
> > +	(*offset)++;
> > +	if (p >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
> >  		return NULL;
> >  	iter = it;
> >  	if (iter->devno == __MAX_SUBCHANNEL) {
> > @@ -314,7 +316,6 @@ struct ccwdev_iter {
> >  			return NULL;
> >  	} else
> >  		iter->devno++;
> > -	(*offset)++;
> >  	return iter;
> >  }
> >  
> >   
> 
> I guess this fixes one aspect:
> "dd: /proc/cio_ignore: cannot skip to specified offset"
> is now gone. So I am tempted to apply this. 

This is definitely an improvement.

> 
> but this code is still fishy:

I'm surprised it took that long; it's been 14 years since I messed
up^W^Wwrote this and there's basically only been a memory leak fix from
you in the meantime... that said, ...

> 
> $ cat /proc/cio_ignore 
> 0.0.fe00-0.0.fefe
> 0.0.ff00-0.0.ffff
> $ dd if=/proc/cio_ignore status=none
> 0.0.fe00-0.0.fefe
> 0.0.ff00-0.0.ffff
> $ dd if=/proc/cio_ignore status=none bs=10
> 0.0.fe00-0.0.fefe
> 0.0.ff00-0.0.ff01-0.0.ff02-0.0.ff03-0.0.ff04-0.0.ff05-0.0.ff06-0.0.ff07-0.0.ff08-0.0.ffff
> $ dd if=/proc/cio_ignore status=none bs=10 skip=1
> .0.fefe
> 0.0.ff00-0.0.ff01-0.0.ff02-0.0.ff03-0.0.ff04-0.0.ff05-0.0.ff06-0.0.ff07-0.0.ff08-0.0.ffff

...what we are doing is translating something that is basically a
per-possible-device value into a range, as otherwise the output would
be quite unreadable for humans. I'm not sure what the semantics should
be if you read in small chunks etc., as the ranges are assembled
on-the-fly.

> Peter, any opinions on this?

I *think* I originally modeled /proc/cio_ignore on a long-gone dasd
procfs file (a very long time before converting it to a seq file); do
we have any other examples of files that do a similar
individual-values-to-ranges conversion?

Re: [PATCH 1/1] cio_ignore_proc_seq_next should increase position index

[Peter Oberparleiter]

On 07.02.2020 14:13, Christian Borntraeger wrote:
> 
> 
> On 24.01.20 06:48, 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>
>> ---
>>  drivers/s390/cio/blacklist.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/s390/cio/blacklist.c b/drivers/s390/cio/blacklist.c
>> index 2a3f874..9cebff8 100644
>> --- a/drivers/s390/cio/blacklist.c
>> +++ b/drivers/s390/cio/blacklist.c
>> @@ -303,8 +303,10 @@ struct ccwdev_iter {
>>  cio_ignore_proc_seq_next(struct seq_file *s, void *it, loff_t *offset)
>>  {
>>  	struct ccwdev_iter *iter;
>> +	loff_t p = *offset;
>>  
>> -	if (*offset >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
>> +	(*offset)++;
>> +	if (p >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
>>  		return NULL;
>>  	iter = it;
>>  	if (iter->devno == __MAX_SUBCHANNEL) {
>> @@ -314,7 +316,6 @@ struct ccwdev_iter {
>>  			return NULL;
>>  	} else
>>  		iter->devno++;
>> -	(*offset)++;
>>  	return iter;
>>  }
>>  
>>
> 
> I guess this fixes one aspect:
> "dd: /proc/cio_ignore: cannot skip to specified offset"
> is now gone. So I am tempted to apply this. 
> 
> but this code is still fishy:
> 
> $ cat /proc/cio_ignore 
> 0.0.fe00-0.0.fefe
> 0.0.ff00-0.0.ffff
> $ dd if=/proc/cio_ignore status=none
> 0.0.fe00-0.0.fefe
> 0.0.ff00-0.0.ffff
> $ dd if=/proc/cio_ignore status=none bs=10
> 0.0.fe00-0.0.fefe
> 0.0.ff00-0.0.ff01-0.0.ff02-0.0.ff03-0.0.ff04-0.0.ff05-0.0.ff06-0.0.ff07-0.0.ff08-0.0.ffff
> $ dd if=/proc/cio_ignore status=none bs=10 skip=1
> .0.fefe
> 0.0.ff00-0.0.ff01-0.0.ff02-0.0.ff03-0.0.ff04-0.0.ff05-0.0.ff06-0.0.ff07-0.0.ff08-0.0.ffff
> 
> 
> Peter, any opinions on this?

A correct implementation of a file read operation must result in the
same data being read independently of whether the file is read in one
go, or if it is read byte-by-byte.

It seems that the current cio_ignore seq-file implementation doesn't
meet that requirement. I don't think that this patch series is the best
way to address this problem though.

My suggestion would be to apply this patch set as is, and then I'll take
the to-do to fix this seq file implementation at a later time.

-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany

Re: [PATCH 1/1] cio_ignore_proc_seq_next should increase position index

[Christian Borntraeger]

On 11.02.20 11:19, Peter Oberparleiter wrote:
[...]
>> but this code is still fishy:
>>
>> $ cat /proc/cio_ignore 
>> 0.0.fe00-0.0.fefe
>> 0.0.ff00-0.0.ffff
>> $ dd if=/proc/cio_ignore status=none
>> 0.0.fe00-0.0.fefe
>> 0.0.ff00-0.0.ffff
>> $ dd if=/proc/cio_ignore status=none bs=10
>> 0.0.fe00-0.0.fefe
>> 0.0.ff00-0.0.ff01-0.0.ff02-0.0.ff03-0.0.ff04-0.0.ff05-0.0.ff06-0.0.ff07-0.0.ff08-0.0.ffff
>> $ dd if=/proc/cio_ignore status=none bs=10 skip=1
>> .0.fefe
>> 0.0.ff00-0.0.ff01-0.0.ff02-0.0.ff03-0.0.ff04-0.0.ff05-0.0.ff06-0.0.ff07-0.0.ff08-0.0.ffff
>>
>>
>> Peter, any opinions on this?
> 
> A correct implementation of a file read operation must result in the
> same data being read independently of whether the file is read in one
> go, or if it is read byte-by-byte.
> 
> It seems that the current cio_ignore seq-file implementation doesn't
> meet that requirement. I don't think that this patch series is the best
> way to address this problem though.
> 
> My suggestion would be to apply this patch set as is, and then I'll take
> the to-do to fix this seq file implementation at a later time.

Ok, I will will Vasily patch. 

Re: [PATCH 1/1] cio_ignore_proc_seq_next should increase position index

[Christian Borntraeger]

On 24.01.20 06:48, 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>

thanks applied. 
> ---
>  drivers/s390/cio/blacklist.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/blacklist.c b/drivers/s390/cio/blacklist.c
> index 2a3f874..9cebff8 100644
> --- a/drivers/s390/cio/blacklist.c
> +++ b/drivers/s390/cio/blacklist.c
> @@ -303,8 +303,10 @@ struct ccwdev_iter {
>  cio_ignore_proc_seq_next(struct seq_file *s, void *it, loff_t *offset)
>  {
>  	struct ccwdev_iter *iter;
> +	loff_t p = *offset;
>  
> -	if (*offset >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
> +	(*offset)++;
> +	if (p >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
>  		return NULL;
>  	iter = it;
>  	if (iter->devno == __MAX_SUBCHANNEL) {
> @@ -314,7 +316,6 @@ struct ccwdev_iter {
>  			return NULL;
>  	} else
>  		iter->devno++;
> -	(*offset)++;
>  	return iter;
>  }
>  
>