[PATCH] resource: make sure requested range intersects root range

[PATCH] resource: make sure requested range intersects root range

[Octavian Purdila]

When the requested and root ranges do not intersect the logic in
__reserve_region_with_split will cause an infinite recursion which
will overflow the stack as seen in the warning bellow.

This particular stack overflow was caused by requesting the
(100000000-107ffffff) range while the root range was (0-ffffffff). In
this case __request_resource would return the whole root range as
conflict range (i.e. 0-ffffffff). Then, the logic in
__reserve_region_with_split would continue the recursion requesting
the new range as (conflict->end+1, end) which incidentally in this
case equals the originally requested range.

This patch aborts looking for a usable range when the requested one is
completely outside the root range to avoid the infinite recursion, and
since this indicates a problem in the layers above, it also prints an
error message indicating the requested and root range in order to make
the problem more easily traceable.

[    5.968374] WARNING: at kernel/sched.c:4129 sub_preempt_count+0x63/0x89()
[    5.975150] Modules linked in:
[    5.978184] Pid: 1, comm: swapper Not tainted 3.0.22-mid27-00004-gb72c817 #46
[    5.985324] Call Trace:
[    5.987759]  [<c1039dfc>] ? console_unlock+0x17b/0x18d
[    5.992891]  [<c1039620>] warn_slowpath_common+0x48/0x5d
[    5.998194]  [<c1031758>] ? sub_preempt_count+0x63/0x89
[    6.003412]  [<c1039644>] warn_slowpath_null+0xf/0x13
[    6.008453]  [<c1031758>] sub_preempt_count+0x63/0x89
[    6.013499]  [<c14d60c4>] _raw_spin_unlock+0x27/0x3f
[    6.018453]  [<c10c6349>] add_partial+0x36/0x3b
[    6.022973]  [<c10c7c0a>] deactivate_slab+0x96/0xb4
[    6.027842]  [<c14cf9d9>] __slab_alloc.isra.54.constprop.63+0x204/0x241
[    6.034456]  [<c103f78f>] ? kzalloc.constprop.5+0x29/0x38
[    6.039842]  [<c103f78f>] ? kzalloc.constprop.5+0x29/0x38
[    6.045232]  [<c10c7dc9>] kmem_cache_alloc_trace+0x51/0xb0
[    6.050710]  [<c103f78f>] ? kzalloc.constprop.5+0x29/0x38
[    6.056100]  [<c103f78f>] kzalloc.constprop.5+0x29/0x38
[    6.061320]  [<c17b45e9>] __reserve_region_with_split+0x1c/0xd1
[    6.067230]  [<c17b4693>] __reserve_region_with_split+0xc6/0xd1
...
[    7.179057]  [<c17b4693>] __reserve_region_with_split+0xc6/0xd1
[    7.184970]  [<c17b4779>] reserve_region_with_split+0x30/0x42
[    7.190709]  [<c17a8ebf>] e820_reserve_resources_late+0xd1/0xe9
[    7.196623]  [<c17c9526>] pcibios_resource_survey+0x23/0x2a
[    7.202184]  [<c17cad8a>] pcibios_init+0x23/0x35
[    7.206789]  [<c17ca574>] pci_subsys_init+0x3f/0x44
[    7.211659]  [<c1002088>] do_one_initcall+0x72/0x122
[    7.216615]  [<c17ca535>] ? pci_legacy_init+0x3d/0x3d
[    7.221659]  [<c17a27ff>] kernel_init+0xa6/0x118
[    7.226265]  [<c17a2759>] ? start_kernel+0x334/0x334
[    7.231223]  [<c14d7482>] kernel_thread_helper+0x6/0x10

Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 kernel/resource.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index e1d2b8e..54402be 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -789,7 +789,13 @@ void __init reserve_region_with_split(struct resource *root,
 		const char *name)
 {
 	write_lock(&resource_lock);
-	__reserve_region_with_split(root, start, end, name);
+	if (start > root->end || end < root->start)
+		pr_err("Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
+		       (unsigned long long)start, (unsigned long long)end,
+		       (unsigned long long)root->start,
+		       (unsigned long long)root->end);
+	else
+		__reserve_region_with_split(root, start, end, name);
 	write_unlock(&resource_lock);
 }
 
-- 
1.7.9.5

Re: [PATCH] resource: make sure requested range intersects root range

[Andrew Morton]

On Sat, 30 Jun 2012 15:00:57 +0300
Octavian Purdila <octavian.purdila@intel.com> wrote:

> When the requested and root ranges do not intersect the logic in
> __reserve_region_with_split will cause an infinite recursion which
> will overflow the stack as seen in the warning bellow.
> 
> This particular stack overflow was caused by requesting the
> (100000000-107ffffff) range while the root range was (0-ffffffff). In
> this case __request_resource would return the whole root range as
> conflict range (i.e. 0-ffffffff). Then, the logic in
> __reserve_region_with_split would continue the recursion requesting
> the new range as (conflict->end+1, end) which incidentally in this
> case equals the originally requested range.
> 
> This patch aborts looking for a usable range when the requested one is
> completely outside the root range to avoid the infinite recursion, and
> since this indicates a problem in the layers above, it also prints an
> error message indicating the requested and root range in order to make
> the problem more easily traceable.

I think we should also emit a stack trace so the faulty caller can be
pinpointed.

> ...
>
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -789,7 +789,13 @@ void __init reserve_region_with_split(struct resource *root,
>  		const char *name)
>  {
>  	write_lock(&resource_lock);
> -	__reserve_region_with_split(root, start, end, name);
> +	if (start > root->end || end < root->start)
> +		pr_err("Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
> +		       (unsigned long long)start, (unsigned long long)end,
> +		       (unsigned long long)root->start,
> +		       (unsigned long long)root->end);
> +	else
> +		__reserve_region_with_split(root, start, end, name);
>  	write_unlock(&resource_lock);
>  }

The fancy way of doing that is

	if (!WARN(start > root->end || end < root->start),
		  "Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
		       (unsigned long long)start, (unsigned long long)end,
		       (unsigned long long)root->start,
		       (unsigned long long)root->end)
		__reserve_region_with_split(root, start, end, name);

but that's quite the eyesore.  How about doing it the simple way?

--- a/kernel/resource.c~resource-make-sure-requested-range-intersects-root-range-fix
+++ a/kernel/resource.c
@@ -792,13 +792,15 @@ void __init reserve_region_with_split(st
 		const char *name)
 {
 	write_lock(&resource_lock);
-	if (start > root->end || end < root->start)
+	if (start > root->end || end < root->start) {
 		pr_err("Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
 		       (unsigned long long)start, (unsigned long long)end,
 		       (unsigned long long)root->start,
 		       (unsigned long long)root->end);
-	else
+		dump_stack();
+	} else {
 		__reserve_region_with_split(root, start, end, name);
+	}
 	write_unlock(&resource_lock);
 }
 
_

Re: [PATCH] resource: make sure requested range intersects root range

[Joe Perches]

On Tue, 2012-07-10 at 14:33 -0700, Andrew Morton wrote:
> On Sat, 30 Jun 2012 15:00:57 +0300
> Octavian Purdila <octavian.purdila@intel.com> wrote:
> 
> > When the requested and root ranges do not intersect the logic in
> > __reserve_region_with_split will cause an infinite recursion which
> > will overflow the stack as seen in the warning bellow.
> > 
> > This particular stack overflow was caused by requesting the
> > (100000000-107ffffff) range while the root range was (0-ffffffff). In
> > this case __request_resource would return the whole root range as
> > conflict range (i.e. 0-ffffffff). Then, the logic in
> > __reserve_region_with_split would continue the recursion requesting
> > the new range as (conflict->end+1, end) which incidentally in this
> > case equals the originally requested range.
> > 
> > This patch aborts looking for a usable range when the requested one is
> > completely outside the root range to avoid the infinite recursion, and
> > since this indicates a problem in the layers above, it also prints an
> > error message indicating the requested and root range in order to make
> > the problem more easily traceable.
> 
> I think we should also emit a stack trace so the faulty caller can be
> pinpointed.
> 
> > ...
> >
> > --- a/kernel/resource.c
> > +++ b/kernel/resource.c
> > @@ -789,7 +789,13 @@ void __init reserve_region_with_split(struct resource *root,
> >  		const char *name)
> >  {
> >  	write_lock(&resource_lock);
> > -	__reserve_region_with_split(root, start, end, name);
> > +	if (start > root->end || end < root->start)
> > +		pr_err("Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
> > +		       (unsigned long long)start, (unsigned long long)end,
> > +		       (unsigned long long)root->start,
> > +		       (unsigned long long)root->end);
> > +	else
> > +		__reserve_region_with_split(root, start, end, name);
> >  	write_unlock(&resource_lock);
> >  }
> 
> The fancy way of doing that is
> 
> 	if (!WARN(start > root->end || end < root->start),
> 		  "Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
> 		       (unsigned long long)start, (unsigned long long)end,
> 		       (unsigned long long)root->start,
> 		       (unsigned long long)root->end)
> 		__reserve_region_with_split(root, start, end, name);
> 
> but that's quite the eyesore.  How about doing it the simple way?
> 
> --- a/kernel/resource.c~resource-make-sure-requested-range-intersects-root-range-fix
> +++ a/kernel/resource.c
> @@ -792,13 +792,15 @@ void __init reserve_region_with_split(st
>  		const char *name)
>  {
>  	write_lock(&resource_lock);
> -	if (start > root->end || end < root->start)
> +	if (start > root->end || end < root->start) {
>  		pr_err("Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",

Maybe use %pr?

		pr_err("Requested range [0x%llx-0x%llx] not in root %pr\n"
		       (unsigned long long)start, (unsigned long long)end,
		       root);

Re: [PATCH] resource: make sure requested range intersects root range

[Ram Pai]

On Tue, Jul 10, 2012 at 02:33:48PM -0700, Andrew Morton wrote:
> On Sat, 30 Jun 2012 15:00:57 +0300
> Octavian Purdila <octavian.purdila@intel.com> wrote:
> 
> > When the requested and root ranges do not intersect the logic in
> > __reserve_region_with_split will cause an infinite recursion which
> > will overflow the stack as seen in the warning bellow.
> > 
> > This particular stack overflow was caused by requesting the
> > (100000000-107ffffff) range while the root range was (0-ffffffff). In
> > this case __request_resource would return the whole root range as
> > conflict range (i.e. 0-ffffffff). Then, the logic in
> > __reserve_region_with_split would continue the recursion requesting
> > the new range as (conflict->end+1, end) which incidentally in this
> > case equals the originally requested range.
> > 
> > This patch aborts looking for a usable range when the requested one is
> > completely outside the root range to avoid the infinite recursion, and
> > since this indicates a problem in the layers above, it also prints an
> > error message indicating the requested and root range in order to make
> > the problem more easily traceable.
> 
> I think we should also emit a stack trace so the faulty caller can be
> pinpointed.
> 
> > ...
> >
> > --- a/kernel/resource.c
> > +++ b/kernel/resource.c
> > @@ -789,7 +789,13 @@ void __init reserve_region_with_split(struct resource *root,
> >  		const char *name)
> >  {
> >  	write_lock(&resource_lock);
> > -	__reserve_region_with_split(root, start, end, name);
> > +	if (start > root->end || end < root->start)
> > +		pr_err("Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
> > +		       (unsigned long long)start, (unsigned long long)end,
> > +		       (unsigned long long)root->start,
> > +		       (unsigned long long)root->end);
> > +	else
> > +		__reserve_region_with_split(root, start, end, name);
> >  	write_unlock(&resource_lock);
> >  }
> 
> The fancy way of doing that is
> 
> 	if (!WARN(start > root->end || end < root->start),
> 		  "Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
> 		       (unsigned long long)start, (unsigned long long)end,
> 		       (unsigned long long)root->start,
> 		       (unsigned long long)root->end)
> 		__reserve_region_with_split(root, start, end, name);
> 
> but that's quite the eyesore.  How about doing it the simple way?
> 
> --- a/kernel/resource.c~resource-make-sure-requested-range-intersects-root-range-fix
> +++ a/kernel/resource.c
> @@ -792,13 +792,15 @@ void __init reserve_region_with_split(st
>  		const char *name)
>  {
>  	write_lock(&resource_lock);
> -	if (start > root->end || end < root->start)
> +	if (start > root->end || end < root->start) {
>  		pr_err("Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
>  		       (unsigned long long)start, (unsigned long long)end,
>  		       (unsigned long long)root->start,
>  		       (unsigned long long)root->end);
> -	else
> +		dump_stack();
> +	} else {
>  		__reserve_region_with_split(root, start, end, name);
> +	}

Wait.. I am not sure this will fix the problem entirely. The above check
will handle the case where the range requested is entirey out of the
root's range.  But if the requested range overlapps that of the root
range, we will still call __reserve_region_with_split() and end up with 
a recursion if there is a overflow. Wont we?


>  	write_unlock(&resource_lock);
>  }
> 
RP

-- 
Ram Pai

Re: [PATCH] resource: make sure requested range intersects root range

[Purdila, Octavian]

On Wed, Jul 11, 2012 at 5:09 AM, Ram Pai <linuxram@us.ibm.com> wrote:

>
> Wait.. I am not sure this will fix the problem entirely. The above check
> will handle the case where the range requested is entirey out of the
> root's range.  But if the requested range overlapps that of the root
> range, we will still call __reserve_region_with_split() and end up with
> a recursion if there is a overflow. Wont we?
>

Good catch. I will fix this as well as address Andrew's and Joe's
comments in a new patch. The only question is how to handle the
overlap case:

(a) abort the whole request or

(b) try to reserve the part that overlaps (and adjust the request to
avoid the overflow)

I think (b) is more in line with the current implementation for reservations.

Re: [PATCH] resource: make sure requested range intersects root range

[Ram Pai]

On Wed, Jul 11, 2012 at 02:06:10PM +0300, Purdila, Octavian wrote:
> On Wed, Jul 11, 2012 at 5:09 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> 
> >
> > Wait.. I am not sure this will fix the problem entirely. The above check
> > will handle the case where the range requested is entirey out of the
> > root's range.  But if the requested range overlapps that of the root
> > range, we will still call __reserve_region_with_split() and end up with
> > a recursion if there is a overflow. Wont we?
> >
> 
> Good catch. I will fix this as well as address Andrew's and Joe's
> comments in a new patch. The only question is how to handle the
> overlap case:
> 
> (a) abort the whole request or
> 
> (b) try to reserve the part that overlaps (and adjust the request to
> avoid the overflow)
> 
> I think (b) is more in line with the current implementation for reservations.


I prefer (b).  following patch should handle that.

diff --git a/kernel/resource.c b/kernel/resource.c
index e1d2b8e..dd87fde 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -780,6 +780,10 @@ static void __init __reserve_region_with_split(struct resource *root,
 
 	if (conflict->start > start)
 		__reserve_region_with_split(root, start, conflict->start-1, name);
+
+	if (conflict->end == parent->end )
+		return;
+
 	if (conflict->end < end)
 		__reserve_region_with_split(root, conflict->end+1, end, name);
 }

RP

Re: [PATCH] resource: make sure requested range intersects root range

[Purdila, Octavian]

On Wed, Jul 11, 2012 at 5:54 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> On Wed, Jul 11, 2012 at 02:06:10PM +0300, Purdila, Octavian wrote:
>> On Wed, Jul 11, 2012 at 5:09 AM, Ram Pai <linuxram@us.ibm.com> wrote:
>>
>> >
>> > Wait.. I am not sure this will fix the problem entirely. The above check
>> > will handle the case where the range requested is entirey out of the
>> > root's range.  But if the requested range overlapps that of the root
>> > range, we will still call __reserve_region_with_split() and end up with
>> > a recursion if there is a overflow. Wont we?
>> >
>>
>> Good catch. I will fix this as well as address Andrew's and Joe's
>> comments in a new patch. The only question is how to handle the
>> overlap case:
>>
>> (a) abort the whole request or
>>
>> (b) try to reserve the part that overlaps (and adjust the request to
>> avoid the overflow)
>>
>> I think (b) is more in line with the current implementation for reservations.
>
>
> I prefer (b).  following patch should handle that.
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index e1d2b8e..dd87fde 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -780,6 +780,10 @@ static void __init __reserve_region_with_split(struct resource *root,
>
>         if (conflict->start > start)
>                 __reserve_region_with_split(root, start, conflict->start-1, name);
> +
> +       if (conflict->end == parent->end )
> +               return;
> +
>         if (conflict->end < end)
>                 __reserve_region_with_split(root, conflict->end+1, end, name);
>  }
>

I don't think this covers all cases, e.g. if root range starts
somewhere above 0 and the request is below the root start point.

What about something like below? It is maybe too verbose, but it
should make it easier to find the offender.

diff --git a/kernel/resource.c b/kernel/resource.c
index e1d2b8e..0d71983 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -788,8 +788,29 @@ void __init reserve_region_with_split(struct
resource *root,
 		resource_size_t start, resource_size_t end,
 		const char *name)
 {
+	int abort = 0;
+
 	write_lock(&resource_lock);
-	__reserve_region_with_split(root, start, end, name);
+	if (!(root->start >= start && root->end >= end)) {
+		pr_err("Requested range (0x%llx-0x%llx) not in root %pr\n",
+		       (unsigned long long)start, (unsigned long long)end,
+		       root);
+		if (start > root->end || end < root->start) {
+			abort = 1;
+			pr_err("Unable to fix request, aborting\n");
+		} else {
+			if (end > root->end)
+				end = root->end;
+			else if (start < root->start)
+				start = root->start;
+			pr_err("Request trimmed to (0x%llx-0x%llx)\n",
+			       (unsigned long long)start,
+			       (unsigned long long)end);
+		}
+		dump_stack();
+	}
+	if (!abort)
+		__reserve_region_with_split(root, start, end, name);
 	write_unlock(&resource_lock);
 }

Re: [PATCH] resource: make sure requested range intersects root range

[Ram Pai]

On Wed, Jul 11, 2012 at 06:26:49PM +0300, Purdila, Octavian wrote:
> On Wed, Jul 11, 2012 at 5:54 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> > On Wed, Jul 11, 2012 at 02:06:10PM +0300, Purdila, Octavian wrote:
> >> On Wed, Jul 11, 2012 at 5:09 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> >>
> >> >
> >> > Wait.. I am not sure this will fix the problem entirely. The above check
> >> > will handle the case where the range requested is entirey out of the
> >> > root's range.  But if the requested range overlapps that of the root
> >> > range, we will still call __reserve_region_with_split() and end up with
> >> > a recursion if there is a overflow. Wont we?
> >> >
> >>
> >> Good catch. I will fix this as well as address Andrew's and Joe's
> >> comments in a new patch. The only question is how to handle the
> >> overlap case:
> >>
> >> (a) abort the whole request or
> >>
> >> (b) try to reserve the part that overlaps (and adjust the request to
> >> avoid the overflow)
> >>
> >> I think (b) is more in line with the current implementation for reservations.
> >
> >
> > I prefer (b).  following patch should handle that.
> >
> > diff --git a/kernel/resource.c b/kernel/resource.c
> > index e1d2b8e..dd87fde 100644
> > --- a/kernel/resource.c
> > +++ b/kernel/resource.c
> > @@ -780,6 +780,10 @@ static void __init __reserve_region_with_split(struct resource *root,
> >
> >         if (conflict->start > start)
> >                 __reserve_region_with_split(root, start, conflict->start-1, name);
> > +
> > +       if (conflict->end == parent->end )
> > +               return;
> > +
> >         if (conflict->end < end)
> >                 __reserve_region_with_split(root, conflict->end+1, end, name);
> >  }
> >
> 
> I don't think this covers all cases, e.g. if root range starts
> somewhere above 0 and the request is below the root start point.

__reserve_region_with_split() is expected to reserve all available
requested range within the root's range. Correct?

If that is the case, the above patch will reserve the range from the
start of the root's range to the request's end? In other words whatever
is overlapping and available. No?

> 
> What about something like below? It is maybe too verbose, but it
> should make it easier to find the offender.
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index e1d2b8e..0d71983 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -788,8 +788,29 @@ void __init reserve_region_with_split(struct
> resource *root,
>  		resource_size_t start, resource_size_t end,
>  		const char *name)
>  {
> +	int abort = 0;
> +
>  	write_lock(&resource_lock);
> -	__reserve_region_with_split(root, start, end, name);
> +	if (!(root->start >= start && root->end >= end)) {

This is checking if the request overlapps with the beginning of 
the root's range?


> +		pr_err("Requested range (0x%llx-0x%llx) not in root %pr\n",
> +		       (unsigned long long)start, (unsigned long long)end,
> +		       root);
> +		if (start > root->end || end < root->start) {

and here it is checking if the requested range has no overlapp with the
root's range, which will always be false.


> +			abort = 1;
> +			pr_err("Unable to fix request, aborting\n");
> +		} else {
> +			if (end > root->end)
> +				end = root->end;
> +			else if (start < root->start)
> +				start = root->start;
> +			pr_err("Request trimmed to (0x%llx-0x%llx)\n",
> +			       (unsigned long long)start,
> +			       (unsigned long long)end);

Yes it is too verbose :), and feels wrong.

> +		}
> +		dump_stack();
> +	}
> +	if (!abort)
> +		__reserve_region_with_split(root, start, end, name);
>  	write_unlock(&resource_lock);
>  }

I think your original patch with Andrew's modification and my above
proposal should solve the problem. 

RP

Re: [PATCH] resource: make sure requested range intersects root range

[Ram Pai]

On Thu, Jul 12, 2012 at 10:02:06AM +0800, Ram Pai wrote:
> On Wed, Jul 11, 2012 at 06:26:49PM +0300, Purdila, Octavian wrote:
> > On Wed, Jul 11, 2012 at 5:54 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> > > On Wed, Jul 11, 2012 at 02:06:10PM +0300, Purdila, Octavian wrote:
> > >> On Wed, Jul 11, 2012 at 5:09 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> > >>
> > >> >
> > >> > Wait.. I am not sure this will fix the problem entirely. The above check
> > >> > will handle the case where the range requested is entirey out of the
> > >> > root's range.  But if the requested range overlapps that of the root
> > >> > range, we will still call __reserve_region_with_split() and end up with
> > >> > a recursion if there is a overflow. Wont we?
> > >> >
> > >>
> > >> Good catch. I will fix this as well as address Andrew's and Joe's
> > >> comments in a new patch. The only question is how to handle the
> > >> overlap case:
> > >>
> > >> (a) abort the whole request or
> > >>
> > >> (b) try to reserve the part that overlaps (and adjust the request to
> > >> avoid the overflow)
> > >>
> > >> I think (b) is more in line with the current implementation for reservations.
> > >
> > >
> > > I prefer (b).  following patch should handle that.
> > >
> > > diff --git a/kernel/resource.c b/kernel/resource.c
> > > index e1d2b8e..dd87fde 100644
> > > --- a/kernel/resource.c
> > > +++ b/kernel/resource.c
> > > @@ -780,6 +780,10 @@ static void __init __reserve_region_with_split(struct resource *root,
> > >
> > >         if (conflict->start > start)
> > >                 __reserve_region_with_split(root, start, conflict->start-1, name);
> > > +
> > > +       if (conflict->end == parent->end )
> > > +               return;
> > > +
> > >         if (conflict->end < end)
> > >                 __reserve_region_with_split(root, conflict->end+1, end, name);
> > >  }
> > >
> > 
> > I don't think this covers all cases, e.g. if root range starts
> > somewhere above 0 and the request is below the root start point.
> 

Ok. I see your point.  Here is a proposal that incoporates the best of all 
the proposals till now..


diff --git a/kernel/resource.c b/kernel/resource.c
index e1d2b8e..c6f4958 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -789,7 +789,19 @@ void __init reserve_region_with_split(struct resource *root,
 		const char *name)
 {
 	write_lock(&resource_lock);
-	__reserve_region_with_split(root, start, end, name);
+	if (start > root->end || end < root->start) {
+ 		pr_err("Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
+ 		       (unsigned long long)start, (unsigned long long)end,
+ 		       (unsigned long long)root->start,
+ 		       (unsigned long long)root->end);
+		dump_stack();
+	} else {
+		if (start < root->start)
+			start = root->start;
+		if (end > root->end)
+			end = root->end;
+ 		__reserve_region_with_split(root, start, end, name);
+	}
 	write_unlock(&resource_lock);
 }
 

Offcourse; it does not warn when the request is partially out of root's range.
But that should be ok, because its still a valid request.
RP

Re: [PATCH] resource: make sure requested range intersects root range

[Purdila, Octavian]

Adding back people on CC, I accidentally reply-to instead of reply-to-all.

On Thu, Jul 12, 2012 at 7:30 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> On Thu, Jul 12, 2012 at 12:56:08PM +0300, Purdila, Octavian wrote:
>> On Thu, Jul 12, 2012 at 11:56 AM, Ram Pai <linuxram@us.ibm.com> wrote:
>
> ..snip..
>> > Offcourse; it does not warn when the request is partially out of root's range.
>> > But that should be ok, because its still a valid request.
>>
>> Since in 99% of cases requests should be completely inside the root
>> range (in fact no bug has been reported until now on this pretty old
>> piece of code) I think being verbose here is a good thing.
>>
>> This sorts of problems occur only in exotic setups (in my case it was
>> Xen on a machine with PAE enabled but 32bit address space) and
>> silently adjusting the request will make logical bugs in the upper
>> layers very hard to detect.
>>
>> That being said, I'll leave it to you and Andrew decide which version
>> (verbose or non-verbose) is better.
>>
>> This is the last verbose version which fixes a few issues from last
>> post as well as makes a few cosmetic changes:
>
> this version is correct. I am ok with it. Probably with one minor
> simplication as below.
>>
>> ---
>>  kernel/resource.c |   25 ++++++++++++++++++++++++-
>>  1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index e1d2b8e..c0e0fa2 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -7,6 +7,8 @@
>>   * Arbitrary resource management.
>>   */
>>
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>>  #include <linux/export.h>
>>  #include <linux/errno.h>
>>  #include <linux/ioport.h>
>
> these above lines must have crept in by mistake? :)
>

No, this is intentional. Since we now use pr_err I think it useful to
have the messages formatted like:

<3>resource: requested range [0x%llx-0x%llx] not in root %pr


>> @@ -788,8 +790,29 @@ void __init reserve_region_with_split(struct
>> resource *root,
>>               resource_size_t start, resource_size_t end,
>>               const char *name)
>>  {
>> +     int abort = 0;
>> +
>>       write_lock(&resource_lock);
>> -     __reserve_region_with_split(root, start, end, name);
>> +     if (!(root->start <= start && root->end >= end)) {
>
> This can be simplified to
>         if (root->start > start || root->end < end) {
>

Sure, I will do that. Thanks for reviewing, I will follow up with a v2
patch to Andrew.

[PATCH] resource: make sure requested range intersects root range

[Octavian Purdila]

When the requested and root ranges do not intersect,
__reserve_region_with_split will eventually overflow the stack.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 kernel/resource.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 7e8ea66..1f8d698 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -789,7 +789,11 @@ void __init reserve_region_with_split(struct resource *root,
 		const char *name)
 {
 	write_lock(&resource_lock);
-	__reserve_region_with_split(root, start, end, name);
+	if (start > root->end || end < root->start)
+		printk(KERN_ERR "Requested range (0x%llx-0x%llx) not in root range (0x%llx-0x%llx)\n",
+		       start, end, root->start, root->end);
+	else
+		__reserve_region_with_split(root, start, end, name);
 	write_unlock(&resource_lock);
 }
 
-- 
1.7.5.4