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

[Ram Pai <linuxram@us.ibm.com>]

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