[PATCH 0/2] uio: open(), mmap(), close()

[PATCH 0/2] uio: open(), mmap(), close()

[Benedikt Spranger]

After open(), mmap(), close() POSIX 1003.1 mmap() declares the mmaped pointer
valid. Add a warning to the documentation to inform UIO users keep this
behaviour into account.
While the inode parameter of in tree UIO kernel drivers is unused by
open/release hooks, remove teh inode parateter. This would help to process
the release hook in an unmap context.

Benedikt Spranger (2):
  uio: add warning to documentation
  uio: do not expose inode to uio open/release hooks

 Documentation/DocBook/uio-howto.tmpl |   11 ++++++++---
 drivers/uio/uio.c                    |    4 ++--
 include/linux/uio_driver.h           |    4 ++--
 3 files changed, 12 insertions(+), 7 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] uio: add warning to documentation

[Benedikt Spranger]

The documentation has no clear statement to the POSIX 1003.1 mmap()
feature, wich allows open(), mmap(), close() while the mmaped pointer is valid.
The release() hook inveigled driver programmer to activate owermanagement
functuonality in the release hook. This may harm.

Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
---
 Documentation/DocBook/uio-howto.tmpl |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
index ac3d001..59a886d 100644
--- a/Documentation/DocBook/uio-howto.tmpl
+++ b/Documentation/DocBook/uio-howto.tmpl
@@ -499,8 +499,13 @@ device is actually used.
 <listitem><para>
 <varname>int (*release)(struct uio_info *info, struct inode *inode)
 </varname>: Optional. If you define your own
-<function>open()</function>, you will probably also want a custom
+<function>release()</function>, you will probably also want a custom
 <function>release()</function> function.
+</para><para>CAVE: The release hook may be processed, even if a mmap is aktive.
+Disabling clocks or other powermanagement functionality may cause a system
+crash, hangup or other unwanted sideeffects.
+</para><para><emphasis>The mmap() function shall add an extra reference to the file associated with the file descriptor fildes which is not removed by a subsequent close() on that file descriptor. This reference shall be removed when there are no more mappings to the file.</emphasis></para><para>
+<link xlink:href="http://pubs.opengroup.org/onlinepubs/009695399/functions/mmap.html">IEEE Std 1003.1, 2004 Edition, mmap()</link>
 </para></listitem>
 
 <listitem><para>
-- 
1.7.10.4

Re: [PATCH 1/2] uio: add warning to documentation

[Greg KH]

On Wed, Dec 12, 2012 at 12:12:01AM +0100, Benedikt Spranger wrote:
> The documentation has no clear statement to the POSIX 1003.1 mmap()
> feature, wich allows open(), mmap(), close() while the mmaped pointer is valid.
> The release() hook inveigled driver programmer to activate owermanagement
> functuonality in the release hook. This may harm.
> 
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  Documentation/DocBook/uio-howto.tmpl |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
> index ac3d001..59a886d 100644
> --- a/Documentation/DocBook/uio-howto.tmpl
> +++ b/Documentation/DocBook/uio-howto.tmpl
> @@ -499,8 +499,13 @@ device is actually used.
>  <listitem><para>
>  <varname>int (*release)(struct uio_info *info, struct inode *inode)
>  </varname>: Optional. If you define your own
> -<function>open()</function>, you will probably also want a custom
> +<function>release()</function>, you will probably also want a custom
>  <function>release()</function> function.

That sentance no longer makes sense.

> +</para><para>CAVE: The release hook may be processed, even if a mmap is aktive.

Huh?

> +Disabling clocks or other powermanagement functionality may cause a system
> +crash, hangup or other unwanted sideeffects.
> +</para><para><emphasis>The mmap() function shall add an extra reference to the file associated with the file descriptor fildes which is not removed by a subsequent close() on that file descriptor. This reference shall be removed when there are no more mappings to the file.</emphasis></para><para>
> +<link xlink:href="http://pubs.opengroup.org/onlinepubs/009695399/functions/mmap.html">IEEE Std 1003.1, 2004 Edition, mmap()</link>

It's not up to us to document the mmap system call here, you should know
how to use it if you write a program with it, right?

Sorry, this patch isn't acceptable at all.

greg k-h

Re: [PATCH 1/2] uio: add warning to documentation

[Benedikt Spranger]

On Tue, 11 Dec 2012 15:18:16 -0800
Greg KH <gregkh@linuxfoundation.org> wrote:

> > -<function>open()</function>, you will probably also want a custom
> > +<function>release()</function>, you will probably also want a
> > custom <function>release()</function> function. 
> That sentance no longer makes sense.
DUH! will fix...

> > +</para><para>CAVE: The release hook may be processed, even if a
> > mmap is aktive.
> Huh?
OK, think about a user of uio_pdrv_genirq and you did your
powermanagement well. The user open the UIO device, do a mmap() and
close the UIO device. Then he access the given pointer and wonders why
the system is stuck. It is a bad idea to disable clocks on release
while a mmap is active.

> > +Disabling clocks or other powermanagement functionality may cause
> > a system +crash, hangup or other unwanted sideeffects.
> > +</para><para><emphasis>The mmap() function shall add an extra
> > reference to the file associated with the file descriptor fildes
> > which is not removed by a subsequent close() on that file
> > descriptor. This reference shall be removed when there are no more
> > mappings to the file.</emphasis></para><para> +<link
> > xlink:href="http://pubs.opengroup.org/onlinepubs/009695399/functions/mmap.html">IEEE
> > Std 1003.1, 2004 Edition, mmap()</link>
> 
> It's not up to us to document the mmap system call here, you should
> know how to use it if you write a program with it, right?
Its not the user of mmap(), it is for the driver programmer. It is a
bad idea to do every kind of powermanagement function in the release
hook.

Regards 
    Bene

Re: [PATCH 1/2] uio: add warning to documentation

[Greg KH]

On Wed, Dec 12, 2012 at 01:45:34AM +0100, Benedikt Spranger wrote:
> On Tue, 11 Dec 2012 15:18:16 -0800
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > > -<function>open()</function>, you will probably also want a custom
> > > +<function>release()</function>, you will probably also want a
> > > custom <function>release()</function> function. 
> > That sentance no longer makes sense.
> DUH! will fix...
> 
> > > +</para><para>CAVE: The release hook may be processed, even if a
> > > mmap is aktive.
> > Huh?
> OK, think about a user of uio_pdrv_genirq and you did your
> powermanagement well. The user open the UIO device, do a mmap() and
> close the UIO device. Then he access the given pointer and wonders why
> the system is stuck. It is a bad idea to disable clocks on release
> while a mmap is active.

A UIO user is root and can do lots of bad things if they are foolish,
are we supposed to enumerate all of them?  :)

> > > +Disabling clocks or other powermanagement functionality may cause
> > > a system +crash, hangup or other unwanted sideeffects.
> > > +</para><para><emphasis>The mmap() function shall add an extra
> > > reference to the file associated with the file descriptor fildes
> > > which is not removed by a subsequent close() on that file
> > > descriptor. This reference shall be removed when there are no more
> > > mappings to the file.</emphasis></para><para> +<link
> > > xlink:href="http://pubs.opengroup.org/onlinepubs/009695399/functions/mmap.html">IEEE
> > > Std 1003.1, 2004 Edition, mmap()</link>
> > 
> > It's not up to us to document the mmap system call here, you should
> > know how to use it if you write a program with it, right?
> Its not the user of mmap(), it is for the driver programmer. It is a
> bad idea to do every kind of powermanagement function in the release
> hook.

I agree, but how can a driver programmer use that information properly
here?

greg k-h

Re: [PATCH 1/2] uio: add warning to documentation

[Hans J. Koch]

On Tue, Dec 11, 2012 at 03:18:16PM -0800, Greg KH wrote:
> On Wed, Dec 12, 2012 at 12:12:01AM +0100, Benedikt Spranger wrote:
> > The documentation has no clear statement to the POSIX 1003.1 mmap()
> > feature, wich allows open(), mmap(), close() while the mmaped pointer is valid.
> > The release() hook inveigled driver programmer to activate owermanagement
> > functuonality in the release hook. This may harm.
> > 
> > Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> > ---
> >  Documentation/DocBook/uio-howto.tmpl |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
> > index ac3d001..59a886d 100644
> > --- a/Documentation/DocBook/uio-howto.tmpl
> > +++ b/Documentation/DocBook/uio-howto.tmpl
> > @@ -499,8 +499,13 @@ device is actually used.
> >  <listitem><para>
> >  <varname>int (*release)(struct uio_info *info, struct inode *inode)
> >  </varname>: Optional. If you define your own
> > -<function>open()</function>, you will probably also want a custom
> > +<function>release()</function>, you will probably also want a custom
> >  <function>release()</function> function.
> 
> That sentance no longer makes sense.
> 
> > +</para><para>CAVE: The release hook may be processed, even if a mmap is aktive.
> 
> Huh?

I think that's right. You can successfully close() a device while userspace is still
using a mapping. If the driver doesn't prevent it, userspace will fail with a SIGBUS
when accessing the mapping the next time.

> 
> > +Disabling clocks or other powermanagement functionality may cause a system
> > +crash, hangup or other unwanted sideeffects.
> > +</para><para><emphasis>The mmap() function shall add an extra reference to the file associated with the file descriptor fildes which is not removed by a subsequent close() on that file descriptor. This reference shall be removed when there are no more mappings to the file.</emphasis></para><para>
> > +<link xlink:href="http://pubs.opengroup.org/onlinepubs/009695399/functions/mmap.html">IEEE Std 1003.1, 2004 Edition, mmap()</link>
> 
> It's not up to us to document the mmap system call here, you should know
> how to use it if you write a program with it, right?

In general, I agree. But in this case, I don't think that this is an mmap()
feature well known to all programmers (In fact, I wasn't aware of that until
Bene told me).

Thanks,
Hans

Re: [PATCH 1/2] uio: add warning to documentation

[Greg KH]

On Wed, Dec 12, 2012 at 02:56:47AM +0100, Hans J. Koch wrote:
> On Tue, Dec 11, 2012 at 03:18:16PM -0800, Greg KH wrote:
> > On Wed, Dec 12, 2012 at 12:12:01AM +0100, Benedikt Spranger wrote:
> > > The documentation has no clear statement to the POSIX 1003.1 mmap()
> > > feature, wich allows open(), mmap(), close() while the mmaped pointer is valid.
> > > The release() hook inveigled driver programmer to activate owermanagement
> > > functuonality in the release hook. This may harm.
> > > 
> > > Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> > > ---
> > >  Documentation/DocBook/uio-howto.tmpl |    7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
> > > index ac3d001..59a886d 100644
> > > --- a/Documentation/DocBook/uio-howto.tmpl
> > > +++ b/Documentation/DocBook/uio-howto.tmpl
> > > @@ -499,8 +499,13 @@ device is actually used.
> > >  <listitem><para>
> > >  <varname>int (*release)(struct uio_info *info, struct inode *inode)
> > >  </varname>: Optional. If you define your own
> > > -<function>open()</function>, you will probably also want a custom
> > > +<function>release()</function>, you will probably also want a custom
> > >  <function>release()</function> function.
> > 
> > That sentance no longer makes sense.
> > 
> > > +</para><para>CAVE: The release hook may be processed, even if a mmap is aktive.
> > 
> > Huh?
> 
> I think that's right. You can successfully close() a device while userspace is still
> using a mapping. If the driver doesn't prevent it, userspace will fail with a SIGBUS
> when accessing the mapping the next time.

I understand mmap(), I was referring to the language of the wording :)

thanks,

greg k-h

[PATCH 2/2] uio: do not expose inode to uio open/release hooks

[Benedikt Spranger]

The inode parameter is unused by in kernel users of UIO. Also the inode
parameter makes it hard to resolve the existing open(), mmap() and close()
difficulty.

Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
---
 Documentation/DocBook/uio-howto.tmpl |    4 ++--
 drivers/uio/uio.c                    |    4 ++--
 include/linux/uio_driver.h           |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
index 59a886d..589992e 100644
--- a/Documentation/DocBook/uio-howto.tmpl
+++ b/Documentation/DocBook/uio-howto.tmpl
@@ -490,14 +490,14 @@ instead of the built-in one.
 </para></listitem>
 
 <listitem><para>
-<varname>int (*open)(struct uio_info *info, struct inode *inode)
+<varname>int (*open)(struct uio_info *info)
 </varname>: Optional. You might want to have your own
 <function>open()</function>, e.g. to enable interrupts only when your
 device is actually used.
 </para></listitem>
 
 <listitem><para>
-<varname>int (*release)(struct uio_info *info, struct inode *inode)
+<varname>int (*release)(struct uio_info *info)
 </varname>: Optional. If you define your own
 <function>release()</function>, you will probably also want a custom
 <function>release()</function> function.
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..bbffc38 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -465,7 +465,7 @@ static int uio_open(struct inode *inode, struct file *filep)
 	filep->private_data = listener;
 
 	if (idev->info->open) {
-		ret = idev->info->open(idev->info, inode);
+		ret = idev->info->open(idev->info);
 		if (ret)
 			goto err_infoopen;
 	}
@@ -496,7 +496,7 @@ static int uio_release(struct inode *inode, struct file *filep)
 	struct uio_device *idev = listener->dev;
 
 	if (idev->info->release)
-		ret = idev->info->release(idev->info, inode);
+		ret = idev->info->release(idev->info);
 
 	module_put(idev->owner);
 	kfree(listener);
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 1ad4724..ad59ded 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -92,8 +92,8 @@ struct uio_info {
 	void			*priv;
 	irqreturn_t (*handler)(int irq, struct uio_info *dev_info);
 	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
-	int (*open)(struct uio_info *info, struct inode *inode);
-	int (*release)(struct uio_info *info, struct inode *inode);
+	int (*open)(struct uio_info *info);
+	int (*release)(struct uio_info *info);
 	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
 };
 
-- 
1.7.10.4

Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks

[Greg KH]

On Wed, Dec 12, 2012 at 12:12:02AM +0100, Benedikt Spranger wrote:
> The inode parameter is unused by in kernel users of UIO.

Ok.

> Also the inode parameter makes it hard to resolve the existing open(),
> mmap() and close() difficulty.

I don't understand, what do you mean by this?  What is this parameter
causing problems with?

greg k-h

Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks

[Hans J. Koch]

On Tue, Dec 11, 2012 at 03:20:32PM -0800, Greg KH wrote:
> On Wed, Dec 12, 2012 at 12:12:02AM +0100, Benedikt Spranger wrote:
> > The inode parameter is unused by in kernel users of UIO.
> 
> Ok.
> 
> > Also the inode parameter makes it hard to resolve the existing open(),
> > mmap() and close() difficulty.
> 
> I don't understand, what do you mean by this?  What is this parameter
> causing problems with?

The problem is that according to POSIX, it is guaranteed that in userspace
you can do

fd = open("/dev/uio0", ...)
ptr = mmap(...fd...)
close(fd)

with ptr still being valid and useable after that.

Thanks,
Hans

Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks

[Greg KH]

On Wed, Dec 12, 2012 at 02:42:22AM +0100, Hans J. Koch wrote:
> On Tue, Dec 11, 2012 at 03:20:32PM -0800, Greg KH wrote:
> > On Wed, Dec 12, 2012 at 12:12:02AM +0100, Benedikt Spranger wrote:
> > > The inode parameter is unused by in kernel users of UIO.
> > 
> > Ok.
> > 
> > > Also the inode parameter makes it hard to resolve the existing open(),
> > > mmap() and close() difficulty.
> > 
> > I don't understand, what do you mean by this?  What is this parameter
> > causing problems with?
> 
> The problem is that according to POSIX, it is guaranteed that in userspace
> you can do
> 
> fd = open("/dev/uio0", ...)
> ptr = mmap(...fd...)
> close(fd)
> 
> with ptr still being valid and useable after that.

Yes, but what does that have to do with this in-kernel, internal api?

confused,

greg k-h

Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks

[Hans J. Koch]

On Tue, Dec 11, 2012 at 08:46:48PM -0800, Greg KH wrote:
> On Wed, Dec 12, 2012 at 02:42:22AM +0100, Hans J. Koch wrote:
> > On Tue, Dec 11, 2012 at 03:20:32PM -0800, Greg KH wrote:
> > > On Wed, Dec 12, 2012 at 12:12:02AM +0100, Benedikt Spranger wrote:
> > > > The inode parameter is unused by in kernel users of UIO.
> > > 
> > > Ok.
> > > 
> > > > Also the inode parameter makes it hard to resolve the existing open(),
> > > > mmap() and close() difficulty.
> > > 
> > > I don't understand, what do you mean by this?  What is this parameter
> > > causing problems with?
> > 
> > The problem is that according to POSIX, it is guaranteed that in userspace
> > you can do
> > 
> > fd = open("/dev/uio0", ...)
> > ptr = mmap(...fd...)
> > close(fd)
> > 
> > with ptr still being valid and useable after that.
> 
> Yes, but what does that have to do with this in-kernel, internal api?

Ah, OK. You're right, the commit message is confusing.

Bene, it's enough to say we drop the inode parameter because nobody
ever needed it. I cannot see why this also helps with the other problem.

Thanks,
Hans

Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks

[Benedikt Spranger]

Am Wed, 12 Dec 2012 09:50:54 +0100
schrieb "Hans J. Koch" <hjk@hansjkoch.de>:

> On Tue, Dec 11, 2012 at 08:46:48PM -0800, Greg KH wrote:
> > Yes, but what does that have to do with this in-kernel, internal api?
> 
> Ah, OK. You're right, the commit message is confusing.
> 
> Bene, it's enough to say we drop the inode parameter because nobody
> ever needed it.
I am fine with that.

> I cannot see why this also helps with the other problem.
It would help, because we can defer calling the release hook until the
last mmap user is gone. In this case the inode pointer may not be valid
anymore.

Regards
    Bene

Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks

[Greg KH]

On Wed, Dec 12, 2012 at 09:56:16AM +0100, Benedikt Spranger wrote:
> Am Wed, 12 Dec 2012 09:50:54 +0100
> schrieb "Hans J. Koch" <hjk@hansjkoch.de>:
> 
> > On Tue, Dec 11, 2012 at 08:46:48PM -0800, Greg KH wrote:
> > > Yes, but what does that have to do with this in-kernel, internal api?
> > 
> > Ah, OK. You're right, the commit message is confusing.
> > 
> > Bene, it's enough to say we drop the inode parameter because nobody
> > ever needed it.
> I am fine with that.
> 
> > I cannot see why this also helps with the other problem.
> It would help, because we can defer calling the release hook until the
> last mmap user is gone. In this case the inode pointer may not be valid
> anymore.

Which, again, is the same for any in-kernel driver with these types of
callbacks.

greg k-h

Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks

[Hans J. Koch]

On Wed, Dec 12, 2012 at 07:08:18AM -0800, Greg KH wrote:
> On Wed, Dec 12, 2012 at 09:56:16AM +0100, Benedikt Spranger wrote:
> > Am Wed, 12 Dec 2012 09:50:54 +0100
> > schrieb "Hans J. Koch" <hjk@hansjkoch.de>:
> > 
> > > On Tue, Dec 11, 2012 at 08:46:48PM -0800, Greg KH wrote:
> > > > Yes, but what does that have to do with this in-kernel, internal api?
> > > 
> > > Ah, OK. You're right, the commit message is confusing.
> > > 
> > > Bene, it's enough to say we drop the inode parameter because nobody
> > > ever needed it.
> > I am fine with that.
> > 
> > > I cannot see why this also helps with the other problem.
> > It would help, because we can defer calling the release hook until the
> > last mmap user is gone. In this case the inode pointer may not be valid
> > anymore.
> 
> Which, again, is the same for any in-kernel driver with these types of
> callbacks.

Is that a general mmap problem that wants to be fixed?

hjk

> 
> greg k-h
> 

Re: [PATCH 2/2] uio: do not expose inode to uio open/release hooks

[Greg KH]

On Thu, Dec 13, 2012 at 01:08:54AM +0100, Hans J. Koch wrote:
> On Wed, Dec 12, 2012 at 07:08:18AM -0800, Greg KH wrote:
> > On Wed, Dec 12, 2012 at 09:56:16AM +0100, Benedikt Spranger wrote:
> > > Am Wed, 12 Dec 2012 09:50:54 +0100
> > > schrieb "Hans J. Koch" <hjk@hansjkoch.de>:
> > > 
> > > > On Tue, Dec 11, 2012 at 08:46:48PM -0800, Greg KH wrote:
> > > > > Yes, but what does that have to do with this in-kernel, internal api?
> > > > 
> > > > Ah, OK. You're right, the commit message is confusing.
> > > > 
> > > > Bene, it's enough to say we drop the inode parameter because nobody
> > > > ever needed it.
> > > I am fine with that.
> > > 
> > > > I cannot see why this also helps with the other problem.
> > > It would help, because we can defer calling the release hook until the
> > > last mmap user is gone. In this case the inode pointer may not be valid
> > > anymore.
> > 
> > Which, again, is the same for any in-kernel driver with these types of
> > callbacks.
> 
> Is that a general mmap problem that wants to be fixed?

Not that I know of, but I haven't messed around in this area of the
kernel in a long time, sorry.

greg k-h