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

Groups > linux.kernel > #1340826 > 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() Simon Guinot <simon.guinot@sequanux.org> - 2016-02-23 17:20 +0100
    Re: [PATCH] kernel/resource.c: fix muxed resource handling in  __request_region() Jesse Barnes <jbarnes@virtuousgeek.org> - 2016-02-23 18:20 +0100
      Re: [PATCH] kernel/resource.c: fix muxed resource handling in  __request_region() One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-02-23 22:40 +0100
      [PATCH] kernel/resource.c: ensure parent is not freed in __request_region() Simon Guinot <simon.guinot@sequanux.org> - 2016-02-24 15:30 +0100

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

[Multipart message — attachments visible in raw view] — view raw

On Mon, Feb 22, 2016 at 12:46:09PM -0800, Jesse Barnes wrote:
> 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?

Hi Jesse,

The fix is not ugly but only incomplete. And I have to say that the
whole IORESOURCE_MUXED thing is not shiny either :)

The main problem in __request_region() is that we are dropping the
spinlock of the resource list while holding a reference on a resource,
waiting for a muxed resource to become available.

From here, I can see two bugs:

1 - At wake-up, the next __request_resource() iteration will not detect
    a remaining conflict. To work properly, __request_resource() needs
    to be called with a parent of the conflicting resource. Instead we
    are passing the conflicting resource itself...
2 - At wake-up the resource pointer we are holding could have been
    freed. Since we are just about to use this pointer to insert a new
    resource in the linked list, it is broken...

My patch fixes 1 and makes things better for 2.

But I think Linus is right. If at wake-up we use the original parent
resource to check again for a conflict, then we are safe.

If you want, I can propose a such patch.

Note that IORESOURCE_MUXED is mostly used by Super-I/Os drivers. A
Super-I/O is a legacy I/O controller embedded on x86 motherboards. It is
used to connect the low-bandwidth devices such as parallel ports, serial
ports, keyboard controllers, hardware monitoring controllers, GPIO
controllers, etc. While each logical device have its own memory region,
a shared memory region is used for some configuration purpose.
IORESOURCE_MUXED is a convenient way to deal with that. For some code
examples you can look at the superio_* functions in the IT87 drivers:
gpio/gpio-it87.c, hwmon/it87.c and watchdog/it87_wdt.c.

I am not aware of any other users for IORESOURCE_MUXED.

Let me know how you want to go and if you need my help.

Simon

[toc] | [next] | [standalone]

#1340881

On 02/23/2016 08:19 AM, Simon Guinot wrote:
> On Mon, Feb 22, 2016 at 12:46:09PM -0800, Jesse Barnes wrote:
>> 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?
> 
> Hi Jesse,
> 
> The fix is not ugly but only incomplete. And I have to say that the
> whole IORESOURCE_MUXED thing is not shiny either :)

Yeah definitely, I'm not casting stones at you, this whole problem is an
ugly one. :)

As Alan said, muxed is really intended for a pretty limited set of
cases.  The "right" solution is a lot more work than its worth, so we
have this instead, which is fine.  But obviously it's been a little
trickier to put in place than we hoped. :)

> The main problem in __request_region() is that we are dropping the
> spinlock of the resource list while holding a reference on a resource,
> waiting for a muxed resource to become available.
> 
> From here, I can see two bugs:
> 
> 1 - At wake-up, the next __request_resource() iteration will not detect
>     a remaining conflict. To work properly, __request_resource() needs
>     to be called with a parent of the conflicting resource. Instead we
>     are passing the conflicting resource itself...
> 2 - At wake-up the resource pointer we are holding could have been
>     freed. Since we are just about to use this pointer to insert a new
>     resource in the linked list, it is broken...
> 
> My patch fixes 1 and makes things better for 2.
> 
> But I think Linus is right. If at wake-up we use the original parent
> resource to check again for a conflict, then we are safe.
> 
> If you want, I can propose a such patch.
> 
> Note that IORESOURCE_MUXED is mostly used by Super-I/Os drivers. A
> Super-I/O is a legacy I/O controller embedded on x86 motherboards. It is
> used to connect the low-bandwidth devices such as parallel ports, serial
> ports, keyboard controllers, hardware monitoring controllers, GPIO
> controllers, etc. While each logical device have its own memory region,
> a shared memory region is used for some configuration purpose.
> IORESOURCE_MUXED is a convenient way to deal with that. For some code
> examples you can look at the superio_* functions in the IT87 drivers:
> gpio/gpio-it87.c, hwmon/it87.c and watchdog/it87_wdt.c.
> 
> I am not aware of any other users for IORESOURCE_MUXED.
> 
> Let me know how you want to go and if you need my help.

I'd be happy if you proposed a patch.  It would also be nice if we could
somehow more formally limit the MUXED usage to these super I/O devices,
just so other users don't get into trouble thinking it's more general
than it is.

Thanks,
Jesse

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

#1341043

> > IORESOURCE_MUXED is a convenient way to deal with that. For some code
> > examples you can look at the superio_* functions in the IT87 drivers:
> > gpio/gpio-it87.c, hwmon/it87.c and watchdog/it87_wdt.c.
> > 
> > I am not aware of any other users for IORESOURCE_MUXED.
> > 
> > Let me know how you want to go and if you need my help.  
> 
> I'd be happy if you proposed a patch.  It would also be nice if we could
> somehow more formally limit the MUXED usage to these super I/O devices,
> just so other users don't get into trouble thinking it's more general
> than it is.

Today I think the problem wouldn't exist. We'd tell the authors of the
drivers to create an mfd or similar to manage the resources and load the
appropriate extra goodies.

Alan

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

#1342067 — [PATCH] kernel/resource.c: ensure parent is not freed in __request_region()

The commit 59ceeaaf355fa0fb16558ef7c24413c804932ada
("kernel/resource.c: fix muxed resource handling in __request_region()")
don't address properly the case where the current parent resource has
been freed while sleeping, waiting for a muxed resource to be available.

This patch fixes the issue by resetting the parent pointer to the root
parent resource when sleeping is needed.

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Cc: stable@vger.kernel.org
---
 kernel/resource.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 3669d1bfc425..14a7f9d66259 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1052,16 +1052,17 @@ static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
 
 /**
  * __request_region - create a new busy resource region
- * @parent: parent resource descriptor
+ * @root: root resource descriptor
  * @start: resource start address
  * @n: resource region size
  * @name: reserving caller's ID string
  * @flags: IO resource flags
  */
-struct resource * __request_region(struct resource *parent,
+struct resource * __request_region(struct resource *root,
 				   resource_size_t start, resource_size_t n,
 				   const char *name, int flags)
 {
+	struct resource *parent = root;
 	DECLARE_WAITQUEUE(wait, current);
 	struct resource *res = alloc_resource(GFP_KERNEL);
 
@@ -1089,6 +1090,7 @@ struct resource * __request_region(struct resource *parent,
 			}
 		}
 		if (conflict->flags & flags & IORESOURCE_MUXED) {
+			parent = root;
 			add_wait_queue(&muxed_resource_wait, &wait);
 			write_unlock(&resource_lock);
 			set_current_state(TASK_UNINTERRUPTIBLE);
-- 
2.1.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web