newbus: Limit units to [0, INT_MAX)

[imp]

Summary

Limit the number of units a newbus device can have to be a positive
number. Reserve and reject the unit INT_MAX so that we can set maxunit
to INT_MAX without ill effect and so the normal signed int math
works. Add sanity checks to make sure we don't get negative unit numbers
from bus routines that can set the unit. Remove now-redundant check for
unit >=0 since it must be after an earlier check.

This should be largely a nop, since we'll likely run out of memory
before we have 2^31 devices. Also, finding unit number is O(n^2) since
we do linear searches for the next unit number, which even on very fast
machines will grind to a halt well before we reach this limit...

Add note to device_find_free_unit that says it can return INT_MAX when
all the unit numbers are in use. The one user in the tree
(ata_pci_attach) will then add a child with this unit and it will fail
and that failure will be handled properly. Hardware limitations, though
mean there will never be more than tens of units, let alone billions.

Update docs to document that EINVAL can be returned for bogus unit
numbers, or when we run out.

Co-Authored-by: Elliott Mitchell <[e-mail]>
Sponsored-by: Netflix

[imp]

finish comment

[jhb]

Maybe cleanup the realloc thing as a prior commit?

sys/kern/subr_bus.c

1239

this comment looks incomplete?

1251–1252

Hmm, is this MIN() needed? We already reject unit == INT_MAX above. Sigh, this expression is ridiculously over-complicated. MINALLOCSIZE is UMA_SMALLEST_UNIT which is 8, so you end up with 'MAX(1, 1)'. I don't care enough about 32-bit kernels to care about the optimization to basically avoid malloc_usable_size(). This should just be:

newsize = unit + 1;

Currently it's a silly way of writing that on 64-bit kernels. On 32-bit kernels it is:

newsizse = roundup2(unit + 1, 2);

This is not a hot path and not worth being unreadable.

1251–1252

This all seems to be an expanded version of realloc. realloc would handle copying the old data, etc. I guess though it doesn't zero the old bits. Using realloc though would mean that all the silliness above with trying to avoid malloc's by simulating malloc_usable_size. This is also something that can use M_WAITOK. :-P

I would write the whole thing now as:

newsize = unit + 1;
dc->devices = reallocf(dc->devices, newsize * sizeof(*newlist), M_BUS, M_WAITOK);
memset(dc->devices + dc->maxunit, 0, sizeof(device_t) * (newsize - dc->maxunit));
dc->maxunit = newsize;

[imp]

sys/kern/subr_bus.c

1239

I noticed and updated while you were reviewing :)

1251–1252

Oh I like that.

1251–1252

Why not dc->devices[unit] = NULL instead of the memset since we're just growing by 1?

[imp]

sys/kern/subr_bus.c

1251–1252

We also allocate 0 of these at first, and grow 1 at a time, so that would be OK. But maybe the memset is safer.

[imp]

tweaks to put the realloc change in front of this.

[imp]

sys/kern/subr_bus.c

1251–1252

Oh wait. If we have wired device 27, then we'll allocate 28 the first time and the old maxunit will be 0....

[jhb]

sys/kern/subr_bus.c

1251–1252

Yeah, I think we have to memset because unit might not just be dc->maxunit.

[imp]

actually closed. Looks the commit with a dash in it didn't work.