Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1338466 > unrolled thread

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] kernel/resource.c: fix muxed resource handling in  __request_region() Vincent Pelletier <plr.vincent@gmail.com> - 2016-02-19 22:10 +0100
    Re: [PATCH] kernel/resource.c: fix muxed resource handling in  __request_region() Jesse Barnes <jbarnes@virtuousgeek.org> - 2016-02-20 00:30 +0100
      Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-20 18:20 +0100
        Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region() Jesse Barnes <jbarnes@virtuousgeek.org> - 2016-02-20 23:30 +0100
          Re: [PATCH] kernel/resource.c: fix muxed resource handling in  __request_region() Alan Cox <alan@linux.intel.com> - 2016-02-22 15:00 +0100
            Re: [PATCH] kernel/resource.c: fix muxed resource handling in  __request_region() Jesse Barnes <jbarnes@virtuousgeek.org> - 2016-02-22 22:00 +0100

#1338466 — Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

Hello,

I finally got around to rebasing some patches, and realised that the
patch from Simon Guinot below still gets rebased over torvalds' v4.4 .

Any reason it was not applied ?
Or was the issue fixed in another, non-git-conflicting way ? (I see
nothing recent in git log kernel/resource.c)

I do not find a trace of a mail confirming that I tested it and that it
fixes the issue. So here goes:
Tested-by: Vincent Pelletier <plr.vincent@gmail.com>

Testing details: bug reproduced on 4.1, patch applied over 4.1 and bug
disappeared. After rebasing this patch (along with others) over 4.4,
bug does not reappear. I did not try to reproduce bug with 4.4, but if
preferred I can give it a go.

On Thu, 10 Sep 2015 00:15:18 +0200, Simon Guinot
<simon.guinot@sequanux.org> wrote:
> In __request_region, if a conflict with a BUSY and MUXED resource is
> detected, then the caller goes to sleep and waits for the resource to
> be released. A pointer on the conflicting resource is kept. At wake-up
> this pointer is used as a parent to retry to request the region. A first
> problem is that this pointer might well be invalid (if for example the
> conflicting resource have already been freed). An another problem is
> that the next call to __request_region() fails to detect a remaining
> conflict. The previously conflicting resource is passed as a parameter
> and __request_region() will look for a conflict among the children of
> this resource and not at the resource itself. It is likely to succeed
> anyway, even if there is still a conflict. Instead, the parent of the
> conflicting resource should be passed to __request_region().
> 
> As a fix attempt, this patch don't update the parent resource pointer in
> the case we have to wait for a muxed region right after.
> 
> Reported-by: Vincent Pelletier <plr.vincent@gmail.com>
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> Tested-by: Vincent Donnefort <vdonnefort@gmail.com>
> ---
>  kernel/resource.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index fed052a1bc9f..b8c84804db6a 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1072,9 +1072,10 @@ struct resource * __request_region(struct resource *parent,
>  		if (!conflict)
>  			break;
>  		if (conflict != parent) {
> -			parent = conflict;
> -			if (!(conflict->flags & IORESOURCE_BUSY))
> +			if (!(conflict->flags & IORESOURCE_BUSY)) {
> +				parent = conflict;
>  				continue;
> +			}
>  		}
>  		if (conflict->flags & flags & IORESOURCE_MUXED) {
>  			add_wait_queue(&muxed_resource_wait, &wait);

Regards,
-- 
Vincent Pelletier

[toc] | [next] | [standalone]

#1338578

+Linus (the de-facto resource guy).

On 02/19/2016 01:10 PM, Vincent Pelletier wrote:
> Hello,
> 
> I finally got around to rebasing some patches, and realised that the
> patch from Simon Guinot below still gets rebased over torvalds' v4.4 .
> 
> Any reason it was not applied ?
> Or was the issue fixed in another, non-git-conflicting way ? (I see
> nothing recent in git log kernel/resource.c)
> 
> I do not find a trace of a mail confirming that I tested it and that it
> fixes the issue. So here goes:
> Tested-by: Vincent Pelletier <plr.vincent@gmail.com>
> 
> Testing details: bug reproduced on 4.1, patch applied over 4.1 and bug
> disappeared. After rebasing this patch (along with others) over 4.4,
> bug does not reappear. I did not try to reproduce bug with 4.4, but if
> preferred I can give it a go.
> 
> On Thu, 10 Sep 2015 00:15:18 +0200, Simon Guinot
> <simon.guinot@sequanux.org> wrote:
>> In __request_region, if a conflict with a BUSY and MUXED resource is
>> detected, then the caller goes to sleep and waits for the resource to
>> be released. A pointer on the conflicting resource is kept. At wake-up
>> this pointer is used as a parent to retry to request the region. A first
>> problem is that this pointer might well be invalid (if for example the
>> conflicting resource have already been freed). An another problem is
>> that the next call to __request_region() fails to detect a remaining
>> conflict. The previously conflicting resource is passed as a parameter
>> and __request_region() will look for a conflict among the children of
>> this resource and not at the resource itself. It is likely to succeed
>> anyway, even if there is still a conflict. Instead, the parent of the
>> conflicting resource should be passed to __request_region().
>>
>> As a fix attempt, this patch don't update the parent resource pointer in
>> the case we have to wait for a muxed region right after.
>>
>> Reported-by: Vincent Pelletier <plr.vincent@gmail.com>
>> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
>> Tested-by: Vincent Donnefort <vdonnefort@gmail.com>
>> ---
>>  kernel/resource.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index fed052a1bc9f..b8c84804db6a 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -1072,9 +1072,10 @@ struct resource * __request_region(struct resource *parent,
>>  		if (!conflict)
>>  			break;
>>  		if (conflict != parent) {
>> -			parent = conflict;
>> -			if (!(conflict->flags & IORESOURCE_BUSY))
>> +			if (!(conflict->flags & IORESOURCE_BUSY)) {
>> +				parent = conflict;
>>  				continue;
>> +			}
>>  		}
>>  		if (conflict->flags & flags & IORESOURCE_MUXED) {
>>  			add_wait_queue(&muxed_resource_wait, &wait);
> 
> Regards,
> 

[toc] | [prev] | [next] | [standalone]

#1338749 — Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

On Fri, Feb 19, 2016 at 3:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> +Linus (the de-facto resource guy).
>
> On 02/19/2016 01:10 PM, Vincent Pelletier wrote:
>>
>> I finally got around to rebasing some patches, and realised that the
>> patch from Simon Guinot below still gets rebased over torvalds' v4.4 .
>>
>> Any reason it was not applied ?
>> Or was the issue fixed in another, non-git-conflicting way ? (I see
>> nothing recent in git log kernel/resource.c)
>>
>> I do not find a trace of a mail confirming that I tested it and that it
>> fixes the issue. So here goes:
>> Tested-by: Vincent Pelletier <plr.vincent@gmail.com>

Hmm.

So I'm not entirely happy with the patch, because I think the problem
with using a possibly free'd parent resource at restart still exists.

As far as I can tell, if we hit the IORESOURCE_MUXED case *after* we
have successfully delved into a resource that wasn't busy, we will
have updated "parent" in a previous iteration of the loop, and we'll
not use the original parent when we then re-start after the sleep. So
quite frankly, I suspect any user of MUXED memory regions is still
fundamentally buggy, and IORESOURCE_MUXED has always been a hacky and
broken thing.

That said, I ended up applying the patch anyway, even if I despise it.
For all I know, muxed users never end up having those non-busy
sub-resources in practice, and maybe there is some serialization at
the top level for the drivers that use it. So if testing has shown
that it helps some actual case, I'll believe the testing. But the code
still looks rather debatable, and the whole IORESOURCE_MUXED approach
looks broken.

Jesse, that came in through you and the drm tree, I think. Alan is
marked as author, and there are other people who actually use and can
test the code. Can you guys think about the code a bit more.

I'm wondering if the *real* fix ends up being to reset the 'parent'
pointer to the original top-level parent after the sleep?

To recap: the patch is in my tree, but I'm not all that happy about it.

                    Linus

[toc] | [prev] | [next] | [standalone]

#1338805 — Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

On February 20, 2016 9:12:01 AM Linus Torvalds 
<torvalds@linux-foundation.org> wrote:

> On Fri, Feb 19, 2016 at 3:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> +Linus (the de-facto resource guy).
>>
>> On 02/19/2016 01:10 PM, Vincent Pelletier wrote:
>>> Tested-by: Vincent Pelletier <plr.vincent@gmail.com>
>
> Hmm.
>
> So I'm not entirely happy with the patch, because I think the problem
> with using a possibly free'd parent resource at restart still exists.
>
> As far as I can tell, if we hit the IORESOURCE_MUXED case *after* we
> have successfully delved into a resource that wasn't busy, we will
> have updated "parent" in a previous iteration of the loop, and we'll
> not use the original parent when we then re-start after the sleep. So
> quite frankly, I suspect any user of MUXED memory regions is still
> fundamentally buggy, and IORESOURCE_MUXED has always been a hacky and
> broken thing.
>
> That said, I ended up applying the patch anyway, even if I despise it.
> For all I know, muxed users never end up having those non-busy
> sub-resources in practice, and maybe there is some serialization at
> the top level for the drivers that use it. So if testing has shown
> that it helps some actual case, I'll believe the testing. But the code
> still looks rather debatable, and the whole IORESOURCE_MUXED approach
> looks broken.
>
> Jesse, that came in through you and the drm tree, I think. Alan is
> marked as author, and there are other people who actually use and can
> test the code. Can you guys think about the code a bit more.
>
> I'm wondering if the *real* fix ends up being to reset the 'parent'
> pointer to the original top-level parent after the sleep?
>
> To recap: the patch is in my tree, but I'm not all that happy about it.

Thanks, yeah i think testing wins in this case.  I'll revisit the muxed 
stuff; I do
remember being dubious at the time, but iirc Alan needed it for something,
and others had been pushing for these sorts of usages for awhile even though
we have some good alternatives in the form of bus and platform drivers that
can manage the appropriate serialization and keep things from stomping
on one another.

(And sorry if this message comes across in some bullshit format, I'm trying
out a new ChromeOS based mail client for fun here.)

Thanks,
Jesse

[toc] | [prev] | [next] | [standalone]

#1339491

> we have some good alternatives in the form of bus and platform
> drivers that
> can manage the appropriate serialization and keep things from
> stomping
> on one another.

It's not used much, especially nowdays. The use case is basically multi
I/O chips on the ISA/LPC bus with magic shared config register ports.

We have sufficiently few of those we could give muxed the boot and
special case them if preferred.

Alan

[toc] | [prev] | [next] | [standalone]

#1339918

On 02/22/2016 05:49 AM, Alan Cox wrote:
>> we have some good alternatives in the form of bus and platform
>> drivers that
>> can manage the appropriate serialization and keep things from
>> stomping
>> on one another.
> 
> It's not used much, especially nowdays. The use case is basically multi
> I/O chips on the ISA/LPC bus with magic shared config register ports.
> 
> We have sufficiently few of those we could give muxed the boot and
> special case them if preferred.

Ah that's right, now I remember the context.  So where should we go from here then?  Just leave the ugly fix in or hack on old stuff and hope not to break it?

Jesse

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web