[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Greg Kroah-Hartman]

On Mon, Jun 12, 2017 at 05:34:59PM -0600, Logan Gunthorpe wrote:
> 1) The test we are looking at is overrunning the dynamic char dev major
> numbers by a lot. On a successful run, it gets down to 216 where the
> limit is 234. Thus, there are 18 too many major numbers assigned and the
> fact that this test used to work is really just luck. (I've cc'd
> developers from 49db08c3 for this issue.)

What test causes so many major numbers to be allocated?  Is this
in-kernel test code?  Do you really have a system that requires so many
different drivers that all want a dynamic char major?

thanks,

gre gk-h

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Logan Gunthorpe]

On 12/06/17 10:18 PM, Greg Kroah-Hartman wrote:
> What test causes so many major numbers to be allocated?  Is this
> in-kernel test code?  Do you really have a system that requires so many
> different drivers that all want a dynamic char major?

This is a 0day kernel robot test. I'm not sure the motivations of its
design but it seems to be similar to an allyesconfig. So all/most
modules are compiled in and allocating their char device regions on boot
of a qemu instance.

Logan

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Greg Kroah-Hartman]

On Mon, Jun 12, 2017 at 10:29:20PM -0600, Logan Gunthorpe wrote:
> 
> 
> On 12/06/17 10:18 PM, Greg Kroah-Hartman wrote:
> > What test causes so many major numbers to be allocated?  Is this
> > in-kernel test code?  Do you really have a system that requires so many
> > different drivers that all want a dynamic char major?
> 
> This is a 0day kernel robot test. I'm not sure the motivations of its
> design but it seems to be similar to an allyesconfig. So all/most
> modules are compiled in and allocating their char device regions on boot
> of a qemu instance.

Ah, that makes sense.  Well, someone can always work on expanding the
range of dynamic char major numbers if they are running out of them on a
real system, I'll gladly take patches for that :)

thanks,

greg k-h

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Greg Kroah-Hartman]

On Tue, Jun 13, 2017 at 06:34:16AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jun 12, 2017 at 10:29:20PM -0600, Logan Gunthorpe wrote:
> > 
> > 
> > On 12/06/17 10:18 PM, Greg Kroah-Hartman wrote:
> > > What test causes so many major numbers to be allocated?  Is this
> > > in-kernel test code?  Do you really have a system that requires so many
> > > different drivers that all want a dynamic char major?
> > 
> > This is a 0day kernel robot test. I'm not sure the motivations of its
> > design but it seems to be similar to an allyesconfig. So all/most
> > modules are compiled in and allocating their char device regions on boot
> > of a qemu instance.
> 
> Ah, that makes sense.  Well, someone can always work on expanding the
> range of dynamic char major numbers if they are running out of them on a
> real system, I'll gladly take patches for that :)

Or better yet, just turn all char major allocations into dynamic, which
would be really good for test systems.  I thought someone proposed
patches for that a long time ago, but I can't find them anymore.  That
would be the simplest solution here.

thnaks,

greg k-h

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Alan Cox]

On Mon, 2017-06-12 at 17:34 -0600, Logan Gunthorpe wrote:
> Hey,
> 
> Ok, I dug into this bug and I know what's going on. Unfortunately,
> it's
> a mess of a couple issues and I don't know what correct the
> solution(s)
> is(are).
> 
> 1) The test we are looking at is overrunning the dynamic char dev
> major
> numbers by a lot. On a successful run, it gets down to 216 where the
> limit is 234. Thus, there are 18 too many major numbers assigned and
> the
> fact that this test used to work is really just luck. (I've cc'd
> developers from 49db08c3 for this issue.)
> 

If you dump the list of majors and drivers do they all look valid or is
there any corruption or repetition that looks odd (wondering if we've
got too many devices or a leak) ?

Alan

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Logan Gunthorpe]

On 13/06/17 05:48 AM, Alan Cox wrote:
> If you dump the list of majors and drivers do they all look valid or is
> there any corruption or repetition that looks odd (wondering if we've
> got too many devices or a leak) ?

They look valid to me.

Logan


Character devices:
  1 mem
  4 /dev/vc/0
  4 tty
  4 ttyS
  5 /dev/tty
  5 /dev/console
  5 /dev/ptmx
  7 vcs
  9 st
 10 misc
 13 input
 21 sg
 29 fb
 43 ttyI
 45 isdn
 68 capi20
 86 ch
 90 mtd
 99 ppdev
108 ppp
128 ptm
136 pts
152 aoechr
153 spi
161 ircomm
172 ttyMX
174 ttyMI
202 cpu/msr
203 cpu/cpuid
204 ttyJ
204 ttyMAX
206 osst
216 rio_cm
217 cros_ec
218 hidraw
219 ttySDIO
220 uio
221 firewire
222 nvme
223 aac
224 mei
225 phantom
226 drm
227 aux
228 cmx
229 cmm
230 ttySLP
231 ttyIPWp
232 ttySL
233 ttySLM
234 noz
235 ttyS
236 ttyRP
237 ttyARC
238 ttyPS
239 ttyIFX
240 ttySC
241 ipmidev
242 rio_mport
243 xz_dec_test
244 bsg
245 pvfs2-req
246 watchdog
247 iio
248 ptp
249 pps
250 dax
251 dimmctl
252 ndctl
253 tpm
254 gpiochip

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Logan Gunthorpe]

On 12/06/17 10:35 PM, Greg Kroah-Hartman wrote:
> Or better yet, just turn all char major allocations into dynamic, which
> would be really good for test systems.  I thought someone proposed
> patches for that a long time ago, but I can't find them anymore.  That
> would be the simplest solution here.

Would people not complain about that? I would not be surprised if some
crazy application is using hard coded major numbers in userspace. So
such a change could potentially break userspace...

Logan

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Greg Kroah-Hartman]

On Tue, Jun 13, 2017 at 10:25:40AM -0600, Logan Gunthorpe wrote:
> 
> 
> On 12/06/17 10:35 PM, Greg Kroah-Hartman wrote:
> > Or better yet, just turn all char major allocations into dynamic, which
> > would be really good for test systems.  I thought someone proposed
> > patches for that a long time ago, but I can't find them anymore.  That
> > would be the simplest solution here.
> 
> Would people not complain about that? I would not be surprised if some
> crazy application is using hard coded major numbers in userspace. So
> such a change could potentially break userspace...

For char devices, I doubt it, but we can't take the chance, which is why
you make it an option.  Then, it's enabled for 'allmodconfig' builds,
which helps testers out.

thanks,

greg k-h

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Logan Gunthorpe]

On 13/06/17 10:35 AM, Greg Kroah-Hartman wrote:
> For char devices, I doubt it, but we can't take the chance, which is why
> you make it an option.  Then, it's enabled for 'allmodconfig' builds,
> which helps testers out.

Well I took a look at this and it looks like a lot of work to modify all
the drivers to support a possible dynamic allocation and I'm not really
able to do all that right now.

However, correct me if I'm missing something, but it looks fairly
straightforward to extend the dynamic region above 256 in cases like
this. There are already fixed major numbers above 255 and the
infrastructure appears to support it. So what are your thoughts on the
change below? I'd be happy to clean it up into a proper patch if you
agree it's a workable option.

Thanks,

Logan



diff --git a/fs/char_dev.c b/fs/char_dev.c
index fb8507f521b2..539352425d95 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -59,6 +59,29 @@ void chrdev_show(struct seq_file *f, off_t offset)

 #endif /* CONFIG_PROC_FS */

+static int find_dynamic_major(void)
+{
+	int i;
+	struct char_device_struct **cp;
+
+	for (i = ARRAY_SIZE(chrdevs)-1; i > CHRDEV_MAJOR_DYN_END; i--) {
+		if (chrdevs[i] == NULL)
+			return i;
+	}
+
+	for (i = CHRDEV_MAJOR_DYN_EXT_START;
+	     i > CHRDEV_MAJOR_DYN_EXT_END; i--) {
+		for (cp = &chrdevs[major_to_index(i)]; *cp; cp = &(*cp)->next)
+			if ((*cp)->major == i)
+				break;
+
+		if (*cp == NULL || (*cp)->major != i)
+			return i;
+	}
+
+	return -EBUSY;
+}
+
 /*
  * Register a single major with a specified minor range.
  *
@@ -84,22 +107,11 @@ __register_chrdev_region(unsigned int major,
unsigned int baseminor,

 	mutex_lock(&chrdevs_lock);

-	/* temporary */
 	if (major == 0) {
-		for (i = ARRAY_SIZE(chrdevs)-1; i > 0; i--) {
-			if (chrdevs[i] == NULL)
-				break;
-		}
-
-		if (i < CHRDEV_MAJOR_DYN_END)
-			pr_warn("CHRDEV \"%s\" major number %d goes below the dynamic
allocation range\n",
-				name, i);
-
-		if (i == 0) {
-			ret = -EBUSY;
+		ret = find_dynamic_major();
+		if (ret < 0)
 			goto out;
-		}
-		major = i;
+		major = ret;
 	}

 	cd->major = major;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 249dad4e8d26..5780d69034ca 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2448,6 +2448,9 @@ static inline void bd_unlink_disk_holder(struct
block_device *bdev,
 #define CHRDEV_MAJOR_HASH_SIZE	255
 /* Marks the bottom of the first segment of free char majors */
 #define CHRDEV_MAJOR_DYN_END 234
+#define CHRDEV_MAJOR_DYN_EXT_START 511
+#define CHRDEV_MAJOR_DYN_EXT_END 384
+
 extern int alloc_chrdev_region(dev_t *, unsigned, unsigned, const char *);
 extern int register_chrdev_region(dev_t, unsigned, const char *);
 extern int __register_chrdev(unsigned int major, unsigned int baseminor,



----

This results in char devices like this (another patch might be prudent
to fix the ordering):

Character devices:
510 ttySLM
  1 mem
511 noz
  4 /dev/vc/0
  4 tty
  4 ttyS
  5 /dev/tty
  5 /dev/console
  5 /dev/ptmx
  7 vcs
  9 st
 10 misc
 13 input
 21 sg
 29 fb
 43 ttyI
 45 isdn
 68 capi20
 86 ch
 90 mtd
 99 ppdev
108 ppp
128 ptm
136 pts
152 aoechr
153 spi
161 ircomm
172 ttyMX
174 ttyMI
202 cpu/msr
203 cpu/cpuid
204 ttyJ
204 ttyMAX
206 osst
226 drm
235 ttyS
236 ttyRP
237 ttyARC
238 ttyPS
239 ttyIFX
494 rio_cm
240 ttySC
495 cros_ec
241 ipmidev
496 hidraw
242 rio_mport
497 ttySDIO
243 xz_dec_test
498 uio
244 bsg
499 firewire
245 pvfs2-req
500 nvme
246 watchdog
501 aac
247 iio
502 mei
248 ptp
503 phantom
249 pps
504 aux
250 dax
505 cmx
251 dimmctl
506 cmm
252 ndctl
507 ttySLP
253 tpm
508 ttyIPWp
254 gpiochip
509 ttySL

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Sven-Haegar Koch]

On Tue, 13 Jun 2017, Greg Kroah-Hartman wrote:

> On Tue, Jun 13, 2017 at 10:25:40AM -0600, Logan Gunthorpe wrote:
> > 
> > On 12/06/17 10:35 PM, Greg Kroah-Hartman wrote:
> > > Or better yet, just turn all char major allocations into dynamic, which
> > > would be really good for test systems.  I thought someone proposed
> > > patches for that a long time ago, but I can't find them anymore.  That
> > > would be the simplest solution here.
> > 
> > Would people not complain about that? I would not be surprised if some
> > crazy application is using hard coded major numbers in userspace. So
> > such a change could potentially break userspace...
> 
> For char devices, I doubt it, but we can't take the chance, which is why
> you make it an option.  Then, it's enabled for 'allmodconfig' builds,
> which helps testers out.

At least for /dev/null, /dev/zero, and perhaps /dev/tty it would 
definitely break things if the major+minor number is not static. I have 
multiple chroot environments having only some minimal needed static 
/dev subdir, with naturally no daemons or filesystem creating those 
on-demand. For the main /dev I use whatever the system sets up, so 
devtmpfs with udev.

c'ya
sven-haegar

-- 
Three may keep a secret, if two of them are dead.
- Ben F.

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Greg Kroah-Hartman]

On Tue, Jun 13, 2017 at 11:47:30AM -0600, Logan Gunthorpe wrote:
> 
> 
> On 13/06/17 10:35 AM, Greg Kroah-Hartman wrote:
> > For char devices, I doubt it, but we can't take the chance, which is why
> > you make it an option.  Then, it's enabled for 'allmodconfig' builds,
> > which helps testers out.
> 
> Well I took a look at this and it looks like a lot of work to modify all
> the drivers to support a possible dynamic allocation and I'm not really
> able to do all that right now.

No, don't modify any drivers, do this in the core chardev code.

> However, correct me if I'm missing something, but it looks fairly
> straightforward to extend the dynamic region above 256 in cases like
> this. There are already fixed major numbers above 255 and the
> infrastructure appears to support it. So what are your thoughts on the
> change below? I'd be happy to clean it up into a proper patch if you
> agree it's a workable option.
> 
> Thanks,
> 
> Logan
> 
> 
> 
> diff --git a/fs/char_dev.c b/fs/char_dev.c
> index fb8507f521b2..539352425d95 100644
> --- a/fs/char_dev.c
> +++ b/fs/char_dev.c
> @@ -59,6 +59,29 @@ void chrdev_show(struct seq_file *f, off_t offset)
> 
>  #endif /* CONFIG_PROC_FS */
> 
> +static int find_dynamic_major(void)
> +{
> +	int i;
> +	struct char_device_struct **cp;
> +
> +	for (i = ARRAY_SIZE(chrdevs)-1; i > CHRDEV_MAJOR_DYN_END; i--) {
> +		if (chrdevs[i] == NULL)
> +			return i;
> +	}
> +
> +	for (i = CHRDEV_MAJOR_DYN_EXT_START;
> +	     i > CHRDEV_MAJOR_DYN_EXT_END; i--) {
> +		for (cp = &chrdevs[major_to_index(i)]; *cp; cp = &(*cp)->next)
> +			if ((*cp)->major == i)
> +				break;
> +
> +		if (*cp == NULL || (*cp)->major != i)
> +			return i;
> +	}
> +
> +	return -EBUSY;
> +}
> +
>  /*
>   * Register a single major with a specified minor range.
>   *
> @@ -84,22 +107,11 @@ __register_chrdev_region(unsigned int major,
> unsigned int baseminor,
> 
>  	mutex_lock(&chrdevs_lock);
> 
> -	/* temporary */
>  	if (major == 0) {
> -		for (i = ARRAY_SIZE(chrdevs)-1; i > 0; i--) {
> -			if (chrdevs[i] == NULL)
> -				break;
> -		}
> -
> -		if (i < CHRDEV_MAJOR_DYN_END)
> -			pr_warn("CHRDEV \"%s\" major number %d goes below the dynamic
> allocation range\n",
> -				name, i);
> -
> -		if (i == 0) {
> -			ret = -EBUSY;
> +		ret = find_dynamic_major();
> +		if (ret < 0)
>  			goto out;
> -		}
> -		major = i;
> +		major = ret;
>  	}
> 
>  	cd->major = major;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 249dad4e8d26..5780d69034ca 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2448,6 +2448,9 @@ static inline void bd_unlink_disk_holder(struct
> block_device *bdev,
>  #define CHRDEV_MAJOR_HASH_SIZE	255
>  /* Marks the bottom of the first segment of free char majors */
>  #define CHRDEV_MAJOR_DYN_END 234
> +#define CHRDEV_MAJOR_DYN_EXT_START 511
> +#define CHRDEV_MAJOR_DYN_EXT_END 384
> +
>  extern int alloc_chrdev_region(dev_t *, unsigned, unsigned, const char *);
>  extern int register_chrdev_region(dev_t, unsigned, const char *);
>  extern int __register_chrdev(unsigned int major, unsigned int baseminor,
> 
> 
> 
> ----
> 
> This results in char devices like this (another patch might be prudent
> to fix the ordering):
> 
> Character devices:
> 510 ttySLM
>   1 mem
> 511 noz
>   4 /dev/vc/0
>   4 tty
>   4 ttyS
>   5 /dev/tty
>   5 /dev/console
>   5 /dev/ptmx
>   7 vcs
>   9 st
>  10 misc
>  13 input
>  21 sg
>  29 fb
>  43 ttyI
>  45 isdn
>  68 capi20
>  86 ch
>  90 mtd
>  99 ppdev
> 108 ppp
> 128 ptm
> 136 pts
> 152 aoechr
> 153 spi
> 161 ircomm
> 172 ttyMX
> 174 ttyMI
> 202 cpu/msr
> 203 cpu/cpuid
> 204 ttyJ
> 204 ttyMAX
> 206 osst
> 226 drm
> 235 ttyS
> 236 ttyRP
> 237 ttyARC
> 238 ttyPS
> 239 ttyIFX
> 494 rio_cm
> 240 ttySC
> 495 cros_ec
> 241 ipmidev
> 496 hidraw
> 242 rio_mport
> 497 ttySDIO
> 243 xz_dec_test
> 498 uio
> 244 bsg
> 499 firewire
> 245 pvfs2-req
> 500 nvme
> 246 watchdog
> 501 aac
> 247 iio
> 502 mei
> 248 ptp
> 503 phantom
> 249 pps
> 504 aux
> 250 dax
> 505 cmx
> 251 dimmctl
> 506 cmm
> 252 ndctl
> 507 ttySLP
> 253 tpm
> 508 ttyIPWp
> 254 gpiochip
> 509 ttySL
> 

At quick glance, ooks good to me, care to clean it up and make it behind
a config option?

thanks,

greg k-h

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Greg Kroah-Hartman]

On Tue, Jun 13, 2017 at 09:13:32PM +0200, Sven-Haegar Koch wrote:
> On Tue, 13 Jun 2017, Greg Kroah-Hartman wrote:
> 
> > On Tue, Jun 13, 2017 at 10:25:40AM -0600, Logan Gunthorpe wrote:
> > > 
> > > On 12/06/17 10:35 PM, Greg Kroah-Hartman wrote:
> > > > Or better yet, just turn all char major allocations into dynamic, which
> > > > would be really good for test systems.  I thought someone proposed
> > > > patches for that a long time ago, but I can't find them anymore.  That
> > > > would be the simplest solution here.
> > > 
> > > Would people not complain about that? I would not be surprised if some
> > > crazy application is using hard coded major numbers in userspace. So
> > > such a change could potentially break userspace...
> > 
> > For char devices, I doubt it, but we can't take the chance, which is why
> > you make it an option.  Then, it's enabled for 'allmodconfig' builds,
> > which helps testers out.
> 
> At least for /dev/null, /dev/zero, and perhaps /dev/tty it would 
> definitely break things if the major+minor number is not static. I have 
> multiple chroot environments having only some minimal needed static 
> /dev subdir, with naturally no daemons or filesystem creating those 
> on-demand. For the main /dev I use whatever the system sets up, so 
> devtmpfs with udev.

No, it wouldn't be required, it would be an option for those people
using devtmpfs.

thanks,

greg k-h

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Logan Gunthorpe]

On 13/06/17 11:00 PM, Greg Kroah-Hartman wrote:
> No, don't modify any drivers, do this in the core chardev code.

Ok, well then maybe I misunderstood what you originally asked for
because I don't see how you can change a fixed allocation to a dynamic
one without touching the driver code which needs to know the major that
was assigned in the end.

> At quick glance, looks good to me, care to clean it up and make it behind
> a config option?

Sure. However, is a config option really necessary here? As is, the
extended dynamic region will only be used if there are too many chardev
majors and it shouldn't have _any_ effect on users that have a small
number. So it seems like not selecting that option is just telling the
kernel to be broken for no obvious trade-off.

Logan

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Linus Walleij]

On Tue, Jun 13, 2017 at 6:34 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:

> Ah, that makes sense.  Well, someone can always work on expanding the
> range of dynamic char major numbers if they are running out of them on a
> real system, I'll gladly take patches for that :)

I started to take a stab at it at one point and incorporated some feedback
from Torvalds etc, it's here:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=chrdev-warn&id=65e5b1e9eb3f777ab7535b74b490e882eeec79d7

It tries to use all "holes" in the chardev major map to shun in a bit
more devices when we run out of the high dynamic major range.

Making them all dynamic seemed dangerous because I was afraid
of userspace ABI breakage because of old userlands with
static mknod:s.

I lost interest when it turned out that the zeroday QEMU stuff was
generating random machines that have no counterpart in the real
world, and then the exercise seemed a bit academic.

The last failures were due to (AFAICT) some relationship between
major and minor numbers that I didn't untangle.

If there is interest I could try to revive it.

Yours,
Linus Walleij

[Merge tag 'pci-v4.12-changes' of git] 857f864014: BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8

[Logan Gunthorpe]

Hi Linus,

On 14/06/17 03:59 AM, Linus Walleij wrote:
> I started to take a stab at it at one point and incorporated some feedback
> from Torvalds etc, it's here:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=chrdev-warn&id=65e5b1e9eb3f777ab7535b74b490e882eeec79d7

Yes, thanks, I did find your work after a google search. However, seeing
it seems to recover only a handful of numbers with some significant
complexity required it seemed like a stopgap solution anyway. I posted a
proposal in this thread that simply extends the dynamic range into a
region above 256 which seems to work well. This means regular
configurations won't be affected an any configuration requiring more
than 20 dynamic majors will simply end up with high numbers.

So unless anyone can think of a reason why this won't work, it's my
preferred direction.

> Making them all dynamic seemed dangerous because I was afraid
> of userspace ABI breakage because of old userlands with
> static mknod:s.

I agree.

Logan