Re: Change in register_blkdev() behavior
On 01/02/18 07:17 PM, Srivatsa S. Bhat wrote: > Thank you for confirming! I'll send a patch to fix that (and the analogous > case for CHRDEV_MAJOR_DYN_EXT_END). Great! Thanks! >>> >>> for (cd = chrdevs[major_to_index(i)]; cd; cd = cd->next) >>> if (cd->major == i) >>> break; >>> >>> if (cd == NULL || cd->major != i) >>> >>> It seems that this latter condition is unnecessary, as it will never be >>> true. (We'll reach here only if cd == NULL or cd->major == i). >> >> Not quite. chrdevs[] may contain majors that also hit on the hash but don't >> equal 'i'. So the for loop will iterate through all hashes matching 'i' and >> if there is one or more and they all don't match 'i', it will fall through >> the loop and cd will be set to something non-null and not equal to i. >> > > Hmm, the code doesn't appear to be doing that though? The loop's fall > through occurs one past the last entry, when cd == NULL. The only > other way it can exit the loop is if it hits the break statement > (which implies that cd->major == i). So what am I missing? Oh, yup, you're right. Looking at it a second (or third) time, it should be NULL or equal to 'i'. Thanks, Logan
Re: Change in register_blkdev() behavior
On 01/02/18 07:17 PM, Srivatsa S. Bhat wrote: > Thank you for confirming! I'll send a patch to fix that (and the analogous > case for CHRDEV_MAJOR_DYN_EXT_END). Great! Thanks! >>> >>> for (cd = chrdevs[major_to_index(i)]; cd; cd = cd->next) >>> if (cd->major == i) >>> break; >>> >>> if (cd == NULL || cd->major != i) >>> >>> It seems that this latter condition is unnecessary, as it will never be >>> true. (We'll reach here only if cd == NULL or cd->major == i). >> >> Not quite. chrdevs[] may contain majors that also hit on the hash but don't >> equal 'i'. So the for loop will iterate through all hashes matching 'i' and >> if there is one or more and they all don't match 'i', it will fall through >> the loop and cd will be set to something non-null and not equal to i. >> > > Hmm, the code doesn't appear to be doing that though? The loop's fall > through occurs one past the last entry, when cd == NULL. The only > other way it can exit the loop is if it hits the break statement > (which implies that cd->major == i). So what am I missing? Oh, yup, you're right. Looking at it a second (or third) time, it should be NULL or equal to 'i'. Thanks, Logan
Re: Change in register_blkdev() behavior
On 2/1/18 5:10 PM, Logan Gunthorpe wrote:
>
>
> On 01/02/18 05:23 PM, Srivatsa S. Bhat wrote:
>> static int find_dynamic_major(void)
>> {
>> int i;
>> struct char_device_struct *cd;
>>
>> for (i = ARRAY_SIZE(chrdevs)-1; i > CHRDEV_MAJOR_DYN_END; i--) {
>>
>> As far as I can see, _DYN_END is inclusive, so shouldn't this be >= ?
>
> Yes, it looks like _DYN_END should have been inclusive based on the way I
> documented it.
>
Thank you for confirming! I'll send a patch to fix that (and the analogous
case for CHRDEV_MAJOR_DYN_EXT_END).
>>
>> for (cd = chrdevs[major_to_index(i)]; cd; cd = cd->next)
>> if (cd->major == i)
>> break;
>>
>> if (cd == NULL || cd->major != i)
>>
>> It seems that this latter condition is unnecessary, as it will never be
>> true. (We'll reach here only if cd == NULL or cd->major == i).
>
> Not quite. chrdevs[] may contain majors that also hit on the hash but don't
> equal 'i'. So the for loop will iterate through all hashes matching 'i' and
> if there is one or more and they all don't match 'i', it will fall through
> the loop and cd will be set to something non-null and not equal to i.
>
Hmm, the code doesn't appear to be doing that though? The loop's fall
through occurs one past the last entry, when cd == NULL. The only
other way it can exit the loop is if it hits the break statement
(which implies that cd->major == i). So what am I missing?
Regards,
Srivatsa
Re: Change in register_blkdev() behavior
On 2/1/18 5:10 PM, Logan Gunthorpe wrote:
>
>
> On 01/02/18 05:23 PM, Srivatsa S. Bhat wrote:
>> static int find_dynamic_major(void)
>> {
>> int i;
>> struct char_device_struct *cd;
>>
>> for (i = ARRAY_SIZE(chrdevs)-1; i > CHRDEV_MAJOR_DYN_END; i--) {
>>
>> As far as I can see, _DYN_END is inclusive, so shouldn't this be >= ?
>
> Yes, it looks like _DYN_END should have been inclusive based on the way I
> documented it.
>
Thank you for confirming! I'll send a patch to fix that (and the analogous
case for CHRDEV_MAJOR_DYN_EXT_END).
>>
>> for (cd = chrdevs[major_to_index(i)]; cd; cd = cd->next)
>> if (cd->major == i)
>> break;
>>
>> if (cd == NULL || cd->major != i)
>>
>> It seems that this latter condition is unnecessary, as it will never be
>> true. (We'll reach here only if cd == NULL or cd->major == i).
>
> Not quite. chrdevs[] may contain majors that also hit on the hash but don't
> equal 'i'. So the for loop will iterate through all hashes matching 'i' and
> if there is one or more and they all don't match 'i', it will fall through
> the loop and cd will be set to something non-null and not equal to i.
>
Hmm, the code doesn't appear to be doing that though? The loop's fall
through occurs one past the last entry, when cd == NULL. The only
other way it can exit the loop is if it hits the break statement
(which implies that cd->major == i). So what am I missing?
Regards,
Srivatsa
Re: Change in register_blkdev() behavior
On 01/02/18 05:23 PM, Srivatsa S. Bhat wrote:
static int find_dynamic_major(void)
{
int i;
struct char_device_struct *cd;
for (i = ARRAY_SIZE(chrdevs)-1; i > CHRDEV_MAJOR_DYN_END; i--) {
As far as I can see, _DYN_END is inclusive, so shouldn't this be >= ?
Yes, it looks like _DYN_END should have been inclusive based on the way
I documented it.
for (cd = chrdevs[major_to_index(i)]; cd; cd = cd->next)
if (cd->major == i)
break;
if (cd == NULL || cd->major != i)
It seems that this latter condition is unnecessary, as it will never be
true. (We'll reach here only if cd == NULL or cd->major == i).
Not quite. chrdevs[] may contain majors that also hit on the hash but
don't equal 'i'. So the for loop will iterate through all hashes
matching 'i' and if there is one or more and they all don't match 'i',
it will fall through the loop and cd will be set to something non-null
and not equal to i.
Thanks for the review!
Logan
Re: Change in register_blkdev() behavior
On 01/02/18 05:23 PM, Srivatsa S. Bhat wrote:
static int find_dynamic_major(void)
{
int i;
struct char_device_struct *cd;
for (i = ARRAY_SIZE(chrdevs)-1; i > CHRDEV_MAJOR_DYN_END; i--) {
As far as I can see, _DYN_END is inclusive, so shouldn't this be >= ?
Yes, it looks like _DYN_END should have been inclusive based on the way
I documented it.
for (cd = chrdevs[major_to_index(i)]; cd; cd = cd->next)
if (cd->major == i)
break;
if (cd == NULL || cd->major != i)
It seems that this latter condition is unnecessary, as it will never be
true. (We'll reach here only if cd == NULL or cd->major == i).
Not quite. chrdevs[] may contain majors that also hit on the hash but
don't equal 'i'. So the for loop will iterate through all hashes
matching 'i' and if there is one or more and they all don't match 'i',
it will fall through the loop and cd will be set to something non-null
and not equal to i.
Thanks for the review!
Logan
Re: Change in register_blkdev() behavior
On 1/31/18 6:24 AM, Greg KH wrote: > On Tue, Jan 30, 2018 at 04:56:32PM -0800, Srivatsa S. Bhat wrote: >> >> Hi, >> >> Before commit 133d55cdb2f "block: order /proc/devices by major number", >> if register_blkdev() was called with major = [1..UINT_MAX], it used to >> succeed (provided the requested major number was actually free). > > How was LTP calling register_blkdev() with such crazy numbers? > Haha :-) No idea! > Anyway, I agree with Logan, this sounds like something to be resolved in > LTP, as allowing block devices with numbers greater than the number we > really allow seems like an odd requirement :) > Agreed! And thanks for confirming! Regards, Srivatsa
Re: Change in register_blkdev() behavior
On 1/31/18 6:24 AM, Greg KH wrote: > On Tue, Jan 30, 2018 at 04:56:32PM -0800, Srivatsa S. Bhat wrote: >> >> Hi, >> >> Before commit 133d55cdb2f "block: order /proc/devices by major number", >> if register_blkdev() was called with major = [1..UINT_MAX], it used to >> succeed (provided the requested major number was actually free). > > How was LTP calling register_blkdev() with such crazy numbers? > Haha :-) No idea! > Anyway, I agree with Logan, this sounds like something to be resolved in > LTP, as allowing block devices with numbers greater than the number we > really allow seems like an odd requirement :) > Agreed! And thanks for confirming! Regards, Srivatsa
Re: Change in register_blkdev() behavior
Hi Logan,
On 1/30/18 5:26 PM, Logan Gunthorpe wrote:
>
>
> On 30/01/18 05:56 PM, Srivatsa S. Bhat wrote:
>> If the restriction on the major number was intentional, perhaps we
>> should get the LTP testcase modified for kernel versions >= 4.14.
>> Otherwise, we should fix register_blkdev to preserve the old behavior.
>> (I guess the same thing applies to commit 8a932f73e5b "char_dev: order
>> /proc/devices by major number" as well).
>
> The restriction was put in place so the code that prints the devices doesn't
> have to run through every integer in order to print the devices in order.
>
> Given the existing documented fixed numbers in [1] and that future new char
> devices should be using dynamic allocation, this seemed like a reasonable
> restriction.
>
> It would be pretty trivial to increase the limit but, IMO, setting it to
> UINT_MAX seems a bit much. Especially given that a lot of the documentation
> and code still very much has remnants of the 256 limit. (The series that
> included this patch only just expanded the char dynamic range to above 256).
> So, I'd suggest the LTP test should change.
>
Sounds good! Thank you!
By the way, I happened to notice a few minor issues with the
find_dynamic_major() function added by this patch series:
static int find_dynamic_major(void)
{
int i;
struct char_device_struct *cd;
for (i = ARRAY_SIZE(chrdevs)-1; i > CHRDEV_MAJOR_DYN_END; i--) {
As far as I can see, _DYN_END is inclusive, so shouldn't this be >= ?
if (chrdevs[i] == NULL)
return i;
}
for (i = CHRDEV_MAJOR_DYN_EXT_START;
i > CHRDEV_MAJOR_DYN_EXT_END; i--) {
Same here; I believe this should be >=
for (cd = chrdevs[major_to_index(i)]; cd; cd = cd->next)
if (cd->major == i)
break;
if (cd == NULL || cd->major != i)
It seems that this latter condition is unnecessary, as it will never be
true. (We'll reach here only if cd == NULL or cd->major == i).
return i;
}
return -EBUSY;
}
Regards,
Srivatsa
Re: Change in register_blkdev() behavior
Hi Logan,
On 1/30/18 5:26 PM, Logan Gunthorpe wrote:
>
>
> On 30/01/18 05:56 PM, Srivatsa S. Bhat wrote:
>> If the restriction on the major number was intentional, perhaps we
>> should get the LTP testcase modified for kernel versions >= 4.14.
>> Otherwise, we should fix register_blkdev to preserve the old behavior.
>> (I guess the same thing applies to commit 8a932f73e5b "char_dev: order
>> /proc/devices by major number" as well).
>
> The restriction was put in place so the code that prints the devices doesn't
> have to run through every integer in order to print the devices in order.
>
> Given the existing documented fixed numbers in [1] and that future new char
> devices should be using dynamic allocation, this seemed like a reasonable
> restriction.
>
> It would be pretty trivial to increase the limit but, IMO, setting it to
> UINT_MAX seems a bit much. Especially given that a lot of the documentation
> and code still very much has remnants of the 256 limit. (The series that
> included this patch only just expanded the char dynamic range to above 256).
> So, I'd suggest the LTP test should change.
>
Sounds good! Thank you!
By the way, I happened to notice a few minor issues with the
find_dynamic_major() function added by this patch series:
static int find_dynamic_major(void)
{
int i;
struct char_device_struct *cd;
for (i = ARRAY_SIZE(chrdevs)-1; i > CHRDEV_MAJOR_DYN_END; i--) {
As far as I can see, _DYN_END is inclusive, so shouldn't this be >= ?
if (chrdevs[i] == NULL)
return i;
}
for (i = CHRDEV_MAJOR_DYN_EXT_START;
i > CHRDEV_MAJOR_DYN_EXT_END; i--) {
Same here; I believe this should be >=
for (cd = chrdevs[major_to_index(i)]; cd; cd = cd->next)
if (cd->major == i)
break;
if (cd == NULL || cd->major != i)
It seems that this latter condition is unnecessary, as it will never be
true. (We'll reach here only if cd == NULL or cd->major == i).
return i;
}
return -EBUSY;
}
Regards,
Srivatsa
Re: Change in register_blkdev() behavior
On Tue, Jan 30, 2018 at 04:56:32PM -0800, Srivatsa S. Bhat wrote: > > Hi, > > Before commit 133d55cdb2f "block: order /proc/devices by major number", > if register_blkdev() was called with major = [1..UINT_MAX], it used to > succeed (provided the requested major number was actually free). How was LTP calling register_blkdev() with such crazy numbers? Anyway, I agree with Logan, this sounds like something to be resolved in LTP, as allowing block devices with numbers greater than the number we really allow seems like an odd requirement :) thanks, greg k-h
Re: Change in register_blkdev() behavior
On Tue, Jan 30, 2018 at 04:56:32PM -0800, Srivatsa S. Bhat wrote: > > Hi, > > Before commit 133d55cdb2f "block: order /proc/devices by major number", > if register_blkdev() was called with major = [1..UINT_MAX], it used to > succeed (provided the requested major number was actually free). How was LTP calling register_blkdev() with such crazy numbers? Anyway, I agree with Logan, this sounds like something to be resolved in LTP, as allowing block devices with numbers greater than the number we really allow seems like an odd requirement :) thanks, greg k-h
Re: Change in register_blkdev() behavior
On 30/01/18 05:56 PM, Srivatsa S. Bhat wrote: If the restriction on the major number was intentional, perhaps we should get the LTP testcase modified for kernel versions >= 4.14. Otherwise, we should fix register_blkdev to preserve the old behavior. (I guess the same thing applies to commit 8a932f73e5b "char_dev: order /proc/devices by major number" as well). The restriction was put in place so the code that prints the devices doesn't have to run through every integer in order to print the devices in order. Given the existing documented fixed numbers in [1] and that future new char devices should be using dynamic allocation, this seemed like a reasonable restriction. It would be pretty trivial to increase the limit but, IMO, setting it to UINT_MAX seems a bit much. Especially given that a lot of the documentation and code still very much has remnants of the 256 limit. (The series that included this patch only just expanded the char dynamic range to above 256). So, I'd suggest the LTP test should change. Logan [1] Documentation/admin-guide/devices.txt
Re: Change in register_blkdev() behavior
On 30/01/18 05:56 PM, Srivatsa S. Bhat wrote: If the restriction on the major number was intentional, perhaps we should get the LTP testcase modified for kernel versions >= 4.14. Otherwise, we should fix register_blkdev to preserve the old behavior. (I guess the same thing applies to commit 8a932f73e5b "char_dev: order /proc/devices by major number" as well). The restriction was put in place so the code that prints the devices doesn't have to run through every integer in order to print the devices in order. Given the existing documented fixed numbers in [1] and that future new char devices should be using dynamic allocation, this seemed like a reasonable restriction. It would be pretty trivial to increase the limit but, IMO, setting it to UINT_MAX seems a bit much. Especially given that a lot of the documentation and code still very much has remnants of the 256 limit. (The series that included this patch only just expanded the char dynamic range to above 256). So, I'd suggest the LTP test should change. Logan [1] Documentation/admin-guide/devices.txt
