Stable Archive on lore.kernel.org
 help / Atom feed
* [PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region()
@ 2019-02-12  8:47 Chengguang Xu
  2019-02-12  8:47 ` [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices Chengguang Xu
  2019-02-12  9:01 ` [PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region() Greg KH
  0 siblings, 2 replies; 8+ messages in thread
From: Chengguang Xu @ 2019-02-12  8:47 UTC (permalink / raw)
  To: gregkh; +Cc: viro, linux-kernel, stable, Chengguang Xu

Current overlap check of minor range cannot correctly
handle a case which is baseminor < existing baseminor &&
baseminor + minorct > existing baseminor + minorct.
Fix it and meanwhile do some code cleanups.

Fixes: 01d553d0fe9f90 ("Chardev checking of overlapping ranges")
Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
Cc: stable@vger.kernel.org
---
 fs/char_dev.c | 94 ++++++++++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 50 deletions(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index a279c58fe360..b25b1da097d5 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -88,86 +88,80 @@ static int find_dynamic_major(void)
 /*
  * Register a single major with a specified minor range.
  *
- * If major == 0 this functions will dynamically allocate a major and return
- * its number.
- *
- * If major > 0 this function will attempt to reserve the passed range of
- * minors and will return zero on success.
+ * If major == 0 this function will dynamically allocate an unused major,
+ * otherwise attempt to reserve the range of minors with given major.
  *
- * Returns a -ve errno on failure.
  */
 static struct char_device_struct *
 __register_chrdev_region(unsigned int major, unsigned int baseminor,
 			   int minorct, const char *name)
 {
-	struct char_device_struct *cd, **cp;
-	int ret = 0;
+	struct char_device_struct *new, *curr, *prev = NULL;
+	int ret = -EBUSY;
 	int i;
 
-	cd = kzalloc(sizeof(struct char_device_struct), GFP_KERNEL);
-	if (cd == NULL)
+	if (major >= CHRDEV_MAJOR_MAX) {
+		pr_err("CHRDEV \"%s\" major requested (%u) is greater than the maximum (%u)\n",
+		       name, major, CHRDEV_MAJOR_MAX-1);
+		return  ERR_PTR(-EINVAL);
+	}
+
+	if (minorct > MINORMASK + 1 - baseminor) {
+		pr_err("CHRDEV \"%s\" minor range requested (%u-%u) is out of range of maximum range (%u-%u) for a single major\n",
+		       name, baseminor, baseminor + minorct - 1, 0, MINORMASK);
+		return ERR_PTR(-EINVAL);
+	}
+
+	new = kzalloc(sizeof(struct char_device_struct), GFP_KERNEL);
+	if (new == NULL)
 		return ERR_PTR(-ENOMEM);
 
 	mutex_lock(&chrdevs_lock);
 
 	if (major == 0) {
-		ret = find_dynamic_major();
-		if (ret < 0) {
+		major = find_dynamic_major();
+		if (major < 0) {
 			pr_err("CHRDEV \"%s\" dynamic allocation region is full\n",
 			       name);
 			goto out;
 		}
-		major = ret;
 	}
 
-	if (major >= CHRDEV_MAJOR_MAX) {
-		pr_err("CHRDEV \"%s\" major requested (%u) is greater than the maximum (%u)\n",
-		       name, major, CHRDEV_MAJOR_MAX-1);
-		ret = -EINVAL;
-		goto out;
-	}
-
-	cd->major = major;
-	cd->baseminor = baseminor;
-	cd->minorct = minorct;
-	strlcpy(cd->name, name, sizeof(cd->name));
-
 	i = major_to_index(major);
+	for (curr = chrdevs[i]; curr; prev = curr, curr = curr->next) {
+		if (curr->major < major)
+			continue;
 
-	for (cp = &chrdevs[i]; *cp; cp = &(*cp)->next)
-		if ((*cp)->major > major ||
-		    ((*cp)->major == major &&
-		     (((*cp)->baseminor >= baseminor) ||
-		      ((*cp)->baseminor + (*cp)->minorct > baseminor))))
+		if (curr->major > major)
 			break;
 
-	/* Check for overlapping minor ranges.  */
-	if (*cp && (*cp)->major == major) {
-		int old_min = (*cp)->baseminor;
-		int old_max = (*cp)->baseminor + (*cp)->minorct - 1;
-		int new_min = baseminor;
-		int new_max = baseminor + minorct - 1;
+		if (curr->baseminor + curr->minorct <= baseminor)
+			continue;
 
-		/* New driver overlaps from the left.  */
-		if (new_max >= old_min && new_max <= old_max) {
-			ret = -EBUSY;
-			goto out;
-		}
+		if (curr->baseminor >= baseminor + minorct)
+			break;
 
-		/* New driver overlaps from the right.  */
-		if (new_min <= old_max && new_min >= old_min) {
-			ret = -EBUSY;
-			goto out;
-		}
+		goto out;
+	}
+
+	new->major = major;
+	new->baseminor = baseminor;
+	new->minorct = minorct;
+	strlcpy(new->name, name, sizeof(new->name));
+
+	if (!prev) {
+		new->next = curr;
+		chrdevs[i] = new;
+	} else {
+		new->next = prev->next;
+		prev->next = new;
 	}
 
-	cd->next = *cp;
-	*cp = cd;
 	mutex_unlock(&chrdevs_lock);
-	return cd;
+	return new;
 out:
 	mutex_unlock(&chrdevs_lock);
-	kfree(cd);
+	kfree(new);
 	return ERR_PTR(ret);
 }
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices
  2019-02-12  8:47 [PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region() Chengguang Xu
@ 2019-02-12  8:47 ` Chengguang Xu
  2019-02-12  9:02   ` Greg KH
  2019-02-12  9:01 ` [PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region() Greg KH
  1 sibling, 1 reply; 8+ messages in thread
From: Chengguang Xu @ 2019-02-12  8:47 UTC (permalink / raw)
  To: gregkh; +Cc: viro, linux-kernel, stable, Chengguang Xu

Currently chardev allows to share major, showing
major with minor range for chardev will be more
helpful.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/char_dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index b25b1da097d5..6f00acdeb308 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -55,7 +55,9 @@ void chrdev_show(struct seq_file *f, off_t offset)
 	mutex_lock(&chrdevs_lock);
 	for (cd = chrdevs[major_to_index(offset)]; cd; cd = cd->next) {
 		if (cd->major == offset)
-			seq_printf(f, "%3d %s\n", cd->major, cd->name);
+			seq_printf(f, "%3d %s (%u-%u)\n", cd->major, cd->name,
+				   cd->baseminor,
+				   cd->baseminor + cd->minorct - 1);
 	}
 	mutex_unlock(&chrdevs_lock);
 }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region()
  2019-02-12  8:47 [PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region() Chengguang Xu
  2019-02-12  8:47 ` [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices Chengguang Xu
@ 2019-02-12  9:01 ` Greg KH
  1 sibling, 0 replies; 8+ messages in thread
From: Greg KH @ 2019-02-12  9:01 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: viro, linux-kernel, stable

On Tue, Feb 12, 2019 at 04:47:38PM +0800, Chengguang Xu wrote:
> Current overlap check of minor range cannot correctly
> handle a case which is baseminor < existing baseminor &&
> baseminor + minorct > existing baseminor + minorct.
> Fix it and meanwhile do some code cleanups.

Ick, don't do a fix and "code cleanups" in the same patch, that makes it
almost impossible to see what the "fix" really was.

That should be two separate patches, please break this up.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices
  2019-02-12  8:47 ` [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices Chengguang Xu
@ 2019-02-12  9:02   ` Greg KH
  2019-02-12 15:18     ` cgxu519
  0 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2019-02-12  9:02 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: viro, linux-kernel, stable

On Tue, Feb 12, 2019 at 04:47:39PM +0800, Chengguang Xu wrote:
> Currently chardev allows to share major, showing
> major with minor range for chardev will be more
> helpful.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
>  fs/char_dev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/char_dev.c b/fs/char_dev.c
> index b25b1da097d5..6f00acdeb308 100644
> --- a/fs/char_dev.c
> +++ b/fs/char_dev.c
> @@ -55,7 +55,9 @@ void chrdev_show(struct seq_file *f, off_t offset)
>  	mutex_lock(&chrdevs_lock);
>  	for (cd = chrdevs[major_to_index(offset)]; cd; cd = cd->next) {
>  		if (cd->major == offset)
> -			seq_printf(f, "%3d %s\n", cd->major, cd->name);
> +			seq_printf(f, "%3d %s (%u-%u)\n", cd->major, cd->name,
> +				   cd->baseminor,
> +				   cd->baseminor + cd->minorct - 1);

You are changing the format of a userspace file, what tools are going to
break when you do this?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices
  2019-02-12  9:02   ` Greg KH
@ 2019-02-12 15:18     ` cgxu519
  2019-02-12 15:20       ` Greg KH
  0 siblings, 1 reply; 8+ messages in thread
From: cgxu519 @ 2019-02-12 15:18 UTC (permalink / raw)
  To: Greg KH; +Cc: viro, linux-kernel, stable

On 2/12/19 5:02 PM, Greg KH wrote:
> On Tue, Feb 12, 2019 at 04:47:39PM +0800, Chengguang Xu wrote:
>> Currently chardev allows to share major, showing
>> major with minor range for chardev will be more
>> helpful.
>>
>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>> ---
>>   fs/char_dev.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/char_dev.c b/fs/char_dev.c
>> index b25b1da097d5..6f00acdeb308 100644
>> --- a/fs/char_dev.c
>> +++ b/fs/char_dev.c
>> @@ -55,7 +55,9 @@ void chrdev_show(struct seq_file *f, off_t offset)
>>   	mutex_lock(&chrdevs_lock);
>>   	for (cd = chrdevs[major_to_index(offset)]; cd; cd = cd->next) {
>>   		if (cd->major == offset)
>> -			seq_printf(f, "%3d %s\n", cd->major, cd->name);
>> +			seq_printf(f, "%3d %s (%u-%u)\n", cd->major, cd->name,
>> +				   cd->baseminor,
>> +				   cd->baseminor + cd->minorct - 1);
> You are changing the format of a userspace file, what tools are going to
> break when you do this?

I'll remove this part in V2. Do you have any idea how to get the minor
range info for particular major? Or adding a similar file to somewhere
under /sys is acceptable?

Thanks




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices
  2019-02-12 15:18     ` cgxu519
@ 2019-02-12 15:20       ` Greg KH
  2019-02-12 15:41         ` cgxu519
  0 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2019-02-12 15:20 UTC (permalink / raw)
  To: cgxu519; +Cc: viro, linux-kernel, stable

On Tue, Feb 12, 2019 at 11:18:22PM +0800, cgxu519 wrote:
> On 2/12/19 5:02 PM, Greg KH wrote:
> > On Tue, Feb 12, 2019 at 04:47:39PM +0800, Chengguang Xu wrote:
> > > Currently chardev allows to share major, showing
> > > major with minor range for chardev will be more
> > > helpful.
> > > 
> > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> > > ---
> > >   fs/char_dev.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/char_dev.c b/fs/char_dev.c
> > > index b25b1da097d5..6f00acdeb308 100644
> > > --- a/fs/char_dev.c
> > > +++ b/fs/char_dev.c
> > > @@ -55,7 +55,9 @@ void chrdev_show(struct seq_file *f, off_t offset)
> > >   	mutex_lock(&chrdevs_lock);
> > >   	for (cd = chrdevs[major_to_index(offset)]; cd; cd = cd->next) {
> > >   		if (cd->major == offset)
> > > -			seq_printf(f, "%3d %s\n", cd->major, cd->name);
> > > +			seq_printf(f, "%3d %s (%u-%u)\n", cd->major, cd->name,
> > > +				   cd->baseminor,
> > > +				   cd->baseminor + cd->minorct - 1);
> > You are changing the format of a userspace file, what tools are going to
> > break when you do this?
> 
> I'll remove this part in V2. Do you have any idea how to get the minor
> range info for particular major? Or adding a similar file to somewhere
> under /sys is acceptable?

Why do you need to know the minor range?  What can userspace do with
this that actually matters?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices
  2019-02-12 15:20       ` Greg KH
@ 2019-02-12 15:41         ` cgxu519
  2019-02-12 18:32           ` Greg KH
  0 siblings, 1 reply; 8+ messages in thread
From: cgxu519 @ 2019-02-12 15:41 UTC (permalink / raw)
  To: Greg KH; +Cc: viro, linux-kernel, stable

On 2/12/19 11:20 PM, Greg KH wrote:
> On Tue, Feb 12, 2019 at 11:18:22PM +0800, cgxu519 wrote:
>> On 2/12/19 5:02 PM, Greg KH wrote:
>>> On Tue, Feb 12, 2019 at 04:47:39PM +0800, Chengguang Xu wrote:
>>>> Currently chardev allows to share major, showing
>>>> major with minor range for chardev will be more
>>>> helpful.
>>>>
>>>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>>>> ---
>>>>    fs/char_dev.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/char_dev.c b/fs/char_dev.c
>>>> index b25b1da097d5..6f00acdeb308 100644
>>>> --- a/fs/char_dev.c
>>>> +++ b/fs/char_dev.c
>>>> @@ -55,7 +55,9 @@ void chrdev_show(struct seq_file *f, off_t offset)
>>>>    	mutex_lock(&chrdevs_lock);
>>>>    	for (cd = chrdevs[major_to_index(offset)]; cd; cd = cd->next) {
>>>>    		if (cd->major == offset)
>>>> -			seq_printf(f, "%3d %s\n", cd->major, cd->name);
>>>> +			seq_printf(f, "%3d %s (%u-%u)\n", cd->major, cd->name,
>>>> +				   cd->baseminor,
>>>> +				   cd->baseminor + cd->minorct - 1);
>>> You are changing the format of a userspace file, what tools are going to
>>> break when you do this?
>> I'll remove this part in V2. Do you have any idea how to get the minor
>> range info for particular major? Or adding a similar file to somewhere
>> under /sys is acceptable?
> Why do you need to know the minor range?  What can userspace do with
> this that actually matters?

Assume that when we try to load a driver module and fail with -EBUSY
because of minor range overlapping, then what can we do for this case?
we even don't know what range has occupied and what range is available.

Also, I think we can obviously notice range overlapping bugs by showing
all registered minor ranges.

Thanks,
Chengguang.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices
  2019-02-12 15:41         ` cgxu519
@ 2019-02-12 18:32           ` Greg KH
  0 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2019-02-12 18:32 UTC (permalink / raw)
  To: cgxu519; +Cc: viro, linux-kernel, stable

On Tue, Feb 12, 2019 at 11:41:35PM +0800, cgxu519 wrote:
> On 2/12/19 11:20 PM, Greg KH wrote:
> > On Tue, Feb 12, 2019 at 11:18:22PM +0800, cgxu519 wrote:
> > > On 2/12/19 5:02 PM, Greg KH wrote:
> > > > On Tue, Feb 12, 2019 at 04:47:39PM +0800, Chengguang Xu wrote:
> > > > > Currently chardev allows to share major, showing
> > > > > major with minor range for chardev will be more
> > > > > helpful.
> > > > > 
> > > > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> > > > > ---
> > > > >    fs/char_dev.c | 4 +++-
> > > > >    1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/char_dev.c b/fs/char_dev.c
> > > > > index b25b1da097d5..6f00acdeb308 100644
> > > > > --- a/fs/char_dev.c
> > > > > +++ b/fs/char_dev.c
> > > > > @@ -55,7 +55,9 @@ void chrdev_show(struct seq_file *f, off_t offset)
> > > > >    	mutex_lock(&chrdevs_lock);
> > > > >    	for (cd = chrdevs[major_to_index(offset)]; cd; cd = cd->next) {
> > > > >    		if (cd->major == offset)
> > > > > -			seq_printf(f, "%3d %s\n", cd->major, cd->name);
> > > > > +			seq_printf(f, "%3d %s (%u-%u)\n", cd->major, cd->name,
> > > > > +				   cd->baseminor,
> > > > > +				   cd->baseminor + cd->minorct - 1);
> > > > You are changing the format of a userspace file, what tools are going to
> > > > break when you do this?
> > > I'll remove this part in V2. Do you have any idea how to get the minor
> > > range info for particular major? Or adding a similar file to somewhere
> > > under /sys is acceptable?
> > Why do you need to know the minor range?  What can userspace do with
> > this that actually matters?
> 
> Assume that when we try to load a driver module and fail with -EBUSY
> because of minor range overlapping, then what can we do for this case?
> we even don't know what range has occupied and what range is available.

Why would drivers ever have minor range overlapping?  Either they are
using a reserved range which is properly documented in the kernel file,
or they are using a dynamic range and that will never overlap as they
are allocated as needed.

So when has this ever happened in the real world?

> Also, I think we can obviously notice range overlapping bugs by showing
> all registered minor ranges.

Again, how can a range overlap?  If so, that's a bug that we can fix
now and has nothing to do with the proc file, which will not show an
overlap as that can not happen at runtime.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-12  8:47 [PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region() Chengguang Xu
2019-02-12  8:47 ` [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices Chengguang Xu
2019-02-12  9:02   ` Greg KH
2019-02-12 15:18     ` cgxu519
2019-02-12 15:20       ` Greg KH
2019-02-12 15:41         ` cgxu519
2019-02-12 18:32           ` Greg KH
2019-02-12  9:01 ` [PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region() Greg KH

Stable Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://lore.kernel.org/stable \
		stable@vger.kernel.org stable@archiver.kernel.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox