Re: [PATCH] Chardev checking of overlapping ranges is incorrect.

[Amos Waterland <apw@us.ibm.com>]

On Tue, Aug 08, 2006 at 08:15:19PM -0500, Linas Vepstas wrote:
> On Tue, Aug 08, 2006 at 06:20:41PM -0400, Amos Waterland wrote:
> > Are you sure?  I do not see how that can happen.  
> 
> On closer examination, indeed, this cannot happen.  I was presuming an
> order dependence when there wasn't one. I am now older and wiser: 
> although I had to write my own patch to become that. 
> 
> I beleive there's still an off-by-one error:
> Presume list contains
>   maj=x, minor=0-64  (and nothing else)
> Go to add
>   maj=x, minor=64-127
> 
> The conditions to break out of the for-loop are never met, so
> the loop terminates naturally, with *cp null, (or with a higher 
> major number), and the new interval is added when it should not 
> have been.

The condition to break out of the for-loop is in fact met, specifically
the check that: (*cp)->baseminor + (*cp)->minorct > baseminor.  In your
example, this is: 0 + 65 > 64.  This is determined by inspection and
verified by putting your proposed sequence through the harness.

Andrew, I believe this is ready for submission.

---

The code in __register_chrdev_region checks that if the driver wishing
to register has the same major as an existing driver the new minor range
is strictly less than the existing minor range.  However, it does not
also check that the new minor range is strictly greater than the
existing minor range.  That is, if driver X has registered with major=x
and minor=0-3, __register_chrdev_region will allow driver Y to register
with major=x and minor=1-4.  

Signed-off-by: Amos Waterland <apw@us.ibm.com>

---

 char_dev.c |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 3483d3c..e13157f 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -109,13 +109,31 @@ __register_chrdev_region(unsigned int ma
 
 	for (cp = &chrdevs[i]; *cp; cp = &(*cp)->next)
 		if ((*cp)->major > major ||
-		    ((*cp)->major == major && (*cp)->baseminor >= baseminor))
+		    ((*cp)->major == major && 
+		     (((*cp)->baseminor >= baseminor) ||
+		      ((*cp)->baseminor + (*cp)->minorct > baseminor))))
 			break;
-	if (*cp && (*cp)->major == major &&
-	    (*cp)->baseminor < baseminor + minorct) {
-		ret = -EBUSY;
-		goto out;
+
+	/* 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;
+
+		/* New driver overlaps from the left.  */
+		if (new_max >= old_min && new_max <= old_max) {
+			ret = -EBUSY;
+			goto out;
+		}
+
+		/* New driver overlaps from the right.  */
+		if (new_min <= old_max && new_min >= old_min) {
+			ret = -EBUSY;
+			goto out;
+		}
 	}
+
 	cd->next = *cp;
 	*cp = cd;
 	mutex_unlock(&chrdevs_lock);