[PATCH v3 1/4] chardev: add additional check for minor range overlap

[PATCH v3 1/4] chardev: add additional check for minor range overlap

[Chengguang Xu]

Current overlap checking cannot correctly handle
a case which is baseminor < existing baseminor &&
baseminor + minorct > existing baseminor + minorct.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
v1->v2:
- Split fix and cleanup patches.
- Remove printing minor range in chrdev_show().

 fs/char_dev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index a279c58fe360..8a63cfa29005 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -159,6 +159,12 @@ __register_chrdev_region(unsigned int major, unsigned int baseminor,
 			ret = -EBUSY;
 			goto out;
 		}
+
+		if (new_min < old_min && new_max > old_max) {
+			ret = -EBUSY;
+			goto out;
+		}
+
 	}

 	cd->next = *cp;
--
2.20.1

[PATCH v3 2/4] chardev: add a check for given minor range

[Chengguang Xu]

register_chrdev_region() carefully checks minor range
before calling __register_chrdev_region() but there is
another path from alloc_chrdev_region() which does not
check the range properly. So add a check for given minor
range in __register_chrdev_region().

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
v1->v2:
- Split fix and cleanup patches.
- Remove printing minor range in chrdev_show().

 fs/char_dev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 8a63cfa29005..6803e98414f1 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -104,6 +104,12 @@ __register_chrdev_region(unsigned int major, unsigned int baseminor,
 	int ret = 0;
 	int i;

+	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);
+	}
+
 	cd = kzalloc(sizeof(struct char_device_struct), GFP_KERNEL);
 	if (cd == NULL)
 		return ERR_PTR(-ENOMEM);
--
2.20.1

[PATCH v3 3/4] chardev: code cleanup for __register_chrdev_region()

[Chengguang Xu]

It's just code cleanup, not functional change.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
v1->v2:
- Split fix and cleanup patches.
- Remove printing minor range in chrdev_show().

v2->v3:
- Set variable ret to '-EBUSY' before checking minor range overlap.

 fs/char_dev.c | 70 +++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 41 deletions(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 6803e98414f1..47fd0561b03d 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -100,10 +100,16 @@ 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 *cd, *curr, *prev = NULL;
+	int ret;
 	int i;

+	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);
@@ -126,55 +132,37 @@ __register_chrdev_region(unsigned int major, unsigned int baseminor,
 		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));
-
+	ret = -EBUSY;
 	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;
+	}

-		if (new_min < old_min && new_max > old_max) {
-			ret = -EBUSY;
-			goto out;
-		}
+	cd->major = major;
+	cd->baseminor = baseminor;
+	cd->minorct = minorct;
+	strlcpy(cd->name, name, sizeof(cd->name));

+	if (!prev) {
+		cd->next = curr;
+		chrdevs[i] = cd;
+	} else {
+		cd->next = prev->next;
+		prev->next = cd;
 	}

-	cd->next = *cp;
-	*cp = cd;
 	mutex_unlock(&chrdevs_lock);
 	return cd;
 out:
--
2.20.1

Re: [PATCH v3 3/4] chardev: code cleanup for __register_chrdev_region()

[Greg KH]

On Fri, Apr 05, 2019 at 07:27:10PM +0800, Chengguang Xu wrote:
> It's just code cleanup, not functional change.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
> v1->v2:
> - Split fix and cleanup patches.
> - Remove printing minor range in chrdev_show().
> 
> v2->v3:
> - Set variable ret to '-EBUSY' before checking minor range overlap.

Can you just send the follow-on patch that does the v3 change as I have
already taken your series.

thanks,

greg k-h

Re: [PATCH v3 3/4] chardev: code cleanup for __register_chrdev_region()

[Chengguang Xu]

> Sent: Friday, April 05, 2019 at 8:32 PM
> From: "Greg KH" <gregkh@linuxfoundation.org>
> To: "Chengguang Xu" <cgxu519@gmx.com>
> Cc: dan.carpenter@oracle.com, linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 3/4] chardev: code cleanup for __register_chrdev_region()
>
> On Fri, Apr 05, 2019 at 07:27:10PM +0800, Chengguang Xu wrote:
> > It's just code cleanup, not functional change.
> >
> > Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> > ---
> > v1->v2:
> > - Split fix and cleanup patches.
> > - Remove printing minor range in chrdev_show().
> >
> > v2->v3:
> > - Set variable ret to '-EBUSY' before checking minor range overlap.
>
> Can you just send the follow-on patch that does the v3 change as I have
> already taken your series.

OK, I'll send another patch for it.

Thanks.
Chengguang.

[PATCH v3 4/4] chardev: update comment based on the code

[Chengguang Xu]

The function comment of __register_chrdev_region()
is out of date, so update it based on the code.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
v1->v2:
- Split fix and cleanup patches.
- Remove printing minor range in chrdev_show().

 fs/char_dev.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 47fd0561b03d..00dfe17871ac 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -88,13 +88,10 @@ 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.
+ * If major > 0 this function will 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,
--
2.20.1

Re: [PATCH v3 1/4] chardev: add additional check for minor range overlap

[Greg KH]

On Fri, Apr 05, 2019 at 07:27:08PM +0800, Chengguang Xu wrote:
> Current overlap checking cannot correctly handle
> a case which is baseminor < existing baseminor &&
> baseminor + minorct > existing baseminor + minorct.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
> v1->v2:
> - Split fix and cleanup patches.
> - Remove printing minor range in chrdev_show().

What is the difference for v3?  And didn't I already take v2 of this
series?

thanks,

greg k-h