Potential bug in queue_count_set

Potential bug in queue_count_set

[Maximilian Luz]

Hi all,

I just stumbled (more or less randomly) over this piece of code
(/drivers/nvme/host/pci.c, v5.1-rc4):

147: static int queue_count_set(const char *val, const struct kernel_param *kp)
148: {
149: 	int n = 0, ret;
150:
151: 	ret = kstrtoint(val, 10, &n);
152: 	if (ret)
153: 		return ret;
154: 	if (n > num_possible_cpus())
155: 		n = num_possible_cpus();
156:
157: 	return param_set_int(val, kp);
158: }

Link with context: https://github.com/torvalds/linux/blob/v5.1-rc4/drivers/nvme/host/pci.c#L154-L155

It looks like lines 154, 155 don't achieve anything as `n` is not being
used afterwards. Sorry if this is already known or a non-issue, just
wanted to let you guys know.

Maximilian

Potential bug in queue_count_set

[Minwoo Im]

On 4/10/19 10:33 PM, Maximilian Luz wrote:
> Hi all,
> 
> I just stumbled (more or less randomly) over this piece of code
> (/drivers/nvme/host/pci.c, v5.1-rc4):
> 
> 147: static int queue_count_set(const char *val, const struct 
> kernel_param *kp)
> 148: {
> 149:???? int n = 0, ret;
> 150:
> 151:???? ret = kstrtoint(val, 10, &n);
> 152:???? if (ret)
> 153:???????? return ret;
> 154:???? if (n > num_possible_cpus())
> 155:???????? n = num_possible_cpus();
> 156:
> 157:???? return param_set_int(val, kp);
> 158: }
> 
> Link with context: 
> https://github.com/torvalds/linux/blob/v5.1-rc4/drivers/nvme/host/pci.c#L154-L155 
> 
> 
> It looks like lines 154, 155 don't achieve anything as `n` is not being
> used afterwards. Sorry if this is already known or a non-issue, just
> wanted to let you guys know.
> 

It looks like module parameter "write_queues" is set to the actual given
value (val) without considering reduced number of queues in case given
value is greater than nr_cpus.

I think that might need sprintf() or something like this to make the
actual value reduced in that case.

> Maximilian
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

Potential bug in queue_count_set

[Minwoo Im]

>>
>> It looks like lines 154, 155 don't achieve anything as `n` is not being
>> used afterwards. Sorry if this is already known or a non-issue, just
>> wanted to let you guys know.
>>
> 
> It looks like module parameter "write_queues" is set to the actual given
> value (val) without considering reduced number of queues in case given
> value is greater than nr_cpus.
> 
> I think that might need sprintf() or something like this to make the
> actual value reduced in that case.


Sorry, the "val" is const so that we cannot go through it with sprintf().
My bad.  This function might return an error if invalid number is given.

Sorry for the confusion.

Have posted patches for it.
   http://lists.infradead.org/pipermail/linux-nvme/2019-April/023369.html

Thanks,

Potential bug in queue_count_set

[Maximilian Luz]

>>>
>>> It looks like lines 154, 155 don't achieve anything as `n` is not being
>>> used afterwards. Sorry if this is already known or a non-issue, just
>>> wanted to let you guys know.
>>>
>>
>> It looks like module parameter "write_queues" is set to the actual given
>> value (val) without considering reduced number of queues in case given
>> value is greater than nr_cpus.
>>
>> I think that might need sprintf() or something like this to make the
>> actual value reduced in that case.
> 
> 
> Sorry, the "val" is const so that we cannot go through it with sprintf().
> My bad.  This function might return an error if invalid number is given.
> 
> Sorry for the confusion.
> 
> Have posted patches for it.
>    http://lists.infradead.org/pipermail/linux-nvme/2019-April/023369.html

I honestly know too little about module-parameters to give a qualified
answer here, but could returning -EINVAL here break things?

 From what I can tell it should be possible to bypass the param_set_int
function and set *(kp->arg) directly, which would reproduce the
param_set_int functionality (kernel/params.c:223).

Alternatively one could argue that after a successful kstrtoint call, we
can strncpy the input into a buffer with fixed length of 11 (10 digits +
null, if I'm not mistaken) and use that as input for param_set_int.

Potential bug in queue_count_set

[Minwoo Im]

> -----Original Message-----
> From: Linux-nvme [mailto:linux-nvme-bounces at lists.infradead.org] On Behalf
> Of Maximilian Luz
> Sent: Friday, April 12, 2019 2:40 AM
> To: Minwoo Im; Keith Busch; Jens Axboe; Christoph Hellwig; Sagi Grimberg
> Cc: linux-nvme at lists.infradead.org
> Subject: Re: Potential bug in queue_count_set
> 
> 
> 
> >>>
> >>> It looks like lines 154, 155 don't achieve anything as `n` is not
> being
> >>> used afterwards. Sorry if this is already known or a non-issue, just
> >>> wanted to let you guys know.
> >>>
> >>
> >> It looks like module parameter "write_queues" is set to the actual
> given
> >> value (val) without considering reduced number of queues in case given
> >> value is greater than nr_cpus.
> >>
> >> I think that might need sprintf() or something like this to make the
> >> actual value reduced in that case.
> >
> >
> > Sorry, the "val" is const so that we cannot go through it with sprintf().
> > My bad.  This function might return an error if invalid number is given.
> >
> > Sorry for the confusion.
> >
> > Have posted patches for it.
> >    http://lists.infradead.org/pipermail/linux-nvme/2019-April/023369.html
> 
> I honestly know too little about module-parameters to give a qualified
> answer here, but could returning -EINVAL here break things?
> 
>  From what I can tell it should be possible to bypass the param_set_int
> function and set *(kp->arg) directly, which would reproduce the
> param_set_int functionality (kernel/params.c:223).

Got what you concern about.  We can make kp->arg have string from "n". But
Anyway we have to give "val" which might be meaningless to param_set_int()
even we modified kp->arg.

> 
> Alternatively one could argue that after a successful kstrtoint call, we
> can strncpy the input into a buffer with fixed length of 11 (10 digits +
> null, if I'm not mistaken) and use that as input for param_set_int.

Right. We can prepare a local buffer to receive the input numeric string.
But I'm not pretty sure what was the intention of Jens's commit 3b6592f70.
I think I need to have a discussion about it, If user gives more than
nr_possible_cpus, then:
  - It should be reduced to a possible value by automatically without
     any error.
  - It should return an error to let user know.

Let's see how they think about it. Also we can have a discussion on that
PATCH mail-loop.

Thanks,
		Minwoo Im

Potential bug in queue_count_set

[Maximilian Luz]

On 4/11/19 11:15 PM, Minwoo Im wrote:
> I think I need to have a discussion about it, If user gives more than
> nr_possible_cpus, then:
>    - It should be reduced to a possible value by automatically without
>       any error.
>    - It should return an error to let user know.
> 
> Let's see how they think about it. Also we can have a discussion on that
> PATCH mail-loop.

Sounds good.

Maximilian