#include <linux/module.h>
#include <linux/init.h>
#include <linux/fs.h>
#include <linux/types.h>
#include <linux/cdev.h>
#define DEVICE_NAME "test"
#define MODULE_NAME "test"
static dev_t Devno;
static struct cdev test_cdev;
static struct mutex test_mutex;
static ssize_t test_read(struct file *filp, char *buff, size_t len,
loff_t *whence)
{
return 1;
}
static ssize_t test_write(struct file *filp, const char *buff,
size_t len, loff_t *whence)
{
return 1;
}
static int test_open(struct inode *ino, struct file *filp)
{
if (mutex_lock_interruptible(&test_mutex))
return -ERESTARTSYS;
printk("%s: test device is open.\n", MODULE_NAME);
return 0;
}
static int test_close(struct inode *ino, struct file *filp)
{
printk("%s: test device is closed.\n", MODULE_NAME);
mutex_unlock(&test_mutex);
return 0;
}
static struct file_operations Fops = { .owner = THIS_MODULE,
.read = test_read,
.write = test_write,
.open = test_open,
.release = test_close };
static int __init test_init(void)
{
int rc;
rc = alloc_chrdev_region(&Devno, 0, 1, DEVICE_NAME);
if (rc < 0)
{
printk("%s: Registration failed...\n",MODULE_NAME);
return rc;
}
printk("%s: Registration %s at major number %d\n", MODULE_NAME,
DEVICE_NAME, MAJOR(Devno));
cdev_init(&test_cdev, &Fops);
test_cdev.owner = THIS_MODULE;
rc = cdev_add(&test_cdev, Devno, 1);
if (rc < 0) {
printk("%s: Device object not added!\n", MODULE_NAME);
unregister_chrdev_region(Devno, 1);
return rc;
}
printk("%s: Device added.\n", MODULE_NAME);
mutex_init(&test_mutex);
return 0;
}
static void __exit test_exit(void)
{
cdev_del(&test_cdev);
unregister_chrdev_region(Devno, 1);
printk("%s: Module removed.\n", MODULE_NAME);
}
module_init(test_init);
module_exit(test_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Frank");
MODULE_DESCRIPTION("Simple char driver.");
On Wed, Mar 12, 2008 at 4:45 PM, Frank Munzert
<[email protected]> wrote:
> we provided a device driver vmur dealing with z/VM virtual unit record
> devices (reader, punch, printer). A corresponding user space tool
> provides functions similar to the CMS commands RECEIVE, PUNCH, PRINT.
> Unit record devices are not meant for concurrent read or write by
> multiple users, that's why we need to serialize access. The driver's
> open method uses mutex_trylock or mutex_lock_interruptible to ensure
> exclusive access to the device, while its release method uses
> mutex_unlock.
snip.
> For the vmur device driver it is crucial to have only one process access
> a given unit record device node at a given time. So having open hold the
> mutex and return to user space is exactly what we want. Is there any
> annotation to tell lockdep to suppress or bypass this kind of warning?
This sounds like a serious abuse of mutexes.
Wouldn't it be correct to use the mutex to protect a separate variable
(which indicates whether the device has been open()ed) and nothing
else? Then there is no need to hold the mutex across the syscalls and
open() can simply fail if the separate variable is set.
Kind regards,
Vegard Nossum
On Wed, 12 Mar 2008, Frank Munzert wrote:
> Unit record devices are not meant for concurrent read or write by
> multiple users, that's why we need to serialize access. The driver's
> open method uses mutex_trylock or mutex_lock_interruptible to ensure
> exclusive access to the device, while its release method uses
> mutex_unlock.
Which is wrong usage of mutex.
> As a consequence, lockdep complains about locks being held when returning to
> user space. We used a very simple char device driver (appended below) to
> produce this message:
> ------------------------------------------------
> testapp/2683 is leaving the kernel with locks still held!
[ ... ]
> For the vmur device driver it is crucial to have only one process access a
> given unit record device node at a given time. So having open hold the mutex
> and return to user space is exactly what we want.
No, it is not. You probably want just to have a single-bit flag somewhere,
and update/check it atomically, see below.
> Is there any annotation to tell lockdep to suppress or bypass this kind
> of warning?
No, as it is a real bug if you use mutexes in this way. What happens if
process that has called open() on your device (and has not closed it yet)
calls fork()?
Another breakage scenario -- what if the filedescriptor is sent through
unix socket to another process? etc.
Look at test_and_set_bit_lock() and clear_bit_unlock(), those you want to
use.
--
Jiri Kosina
SUSE Labs
On Wed, 2008-03-12 at 22:40 +0100, Jiri Kosina wrote:
> No, as it is a real bug if you use mutexes in this way. What happens if
> process that has called open() on your device (and has not closed it yet)
> calls fork()?
> Another breakage scenario -- what if the filedescriptor is sent through
> unix socket to another process? etc.
There's a number of places where a semaphore is used across system
calls.
for instance the usb skeleton,
drivers/usb/usb-skeleton.c
Several of the watchdog drivers,
drivers/watchdog/s3c2410_wdt.c
These need to be removed, but the usage is clearly not compatible with
the mutex API ..
If you convert them to atomic counts then you loose the sleeping aspect
of the semaphore, which you'd have to add back somehow.
The only API that seems straight forward is using complete's .. Then you
get an atomic count and all the sleeping function calls you might want..
(include/linux/completion.h) The problem with complete's is that you
can't start them out at "1" or "completed" unless you actually run
complete() once during initialization (that's kind of ugly) ..
Daniel
On Thu, 13 Mar 2008 08:43:49 -0700, Daniel Walker wrote:
> On Wed, 2008-03-12 at 22:40 +0100, Jiri Kosina wrote:
> > No, as it is a real bug if you use mutexes in this way. What happens if
> > process that has called open() on your device (and has not closed it yet)
> > calls fork()?
> > Another breakage scenario -- what if the filedescriptor is sent through
> > unix socket to another process? etc.
>
> There's a number of places where a semaphore is used across system
> calls.
>
> for instance the usb skeleton,
> drivers/usb/usb-skeleton.c
>
> Several of the watchdog drivers,
> drivers/watchdog/s3c2410_wdt.c
>
> These need to be removed, but the usage is clearly not compatible with
> the mutex API ..
>
> If you convert them to atomic counts then you loose the sleeping aspect
> of the semaphore, which you'd have to add back somehow.
>
> The only API that seems straight forward is using complete's .. Then you
> get an atomic count and all the sleeping function calls you might want..
> (include/linux/completion.h) The problem with complete's is that you
> can't start them out at "1" or "completed" unless you actually run
> complete() once during initialization (that's kind of ugly) ..
>
> Daniel
They are not only bugs in the implementation,
they are bugs in the design or in the definition too!
Assuming the definition of fork in the OpenGroup.org
http://www.opengroup.org/onlinepubs/000095399/functions/fork.html
"The fork() function shall create a new process. The new process
(child process)
shall be an exact copy of the calling process (parent process) except as
detailed below: ....."
Here there are some problems when a process is forked:
* when the process is multithreaded instead of monothreaded => dangerous!
(why? e.g. one of the threads can fork from its own process that will
have again many cloned threads duplicating its forbidden tasks and will
have undefined behaviour because of this forked multithreaded process!)
* the locks have a state: "unlocked" or "locked by one correspondent process",
but when the correspondent process is forked with some its locks locked by
itself then the locks are "locked by 2 or more processes instead one"
violating the theory of locking. There is no theorical solution
for this problem.
(it's not solution that all the locks of the forked process are
unlocked because
its program counter is "inside" of the mutual exclusion of the locked lock)
* even for semaphores, message passing, mutexes in shared memory (IPC), etc.
* when they are servers, it doesn't exist forked listen's ports from sockets!
* when they are clients, they can't duplicate sessions with same
TCP/IP client port!
* they can't duplicate pipes, streams, groups of children processes, etc.
* the same offsets of the opened files (or streams) for writing can imply them
to overwrite characters or blocks of the same opened file.
* etc.
In the fork specification, it almost says e.g. that "the fork shall
not inherit" but
it doesn't say "how to solve it when it's inherited or not inherited"!
Eureka! You did understand the difference when you found an unsolvable bug!
J.C.Pizarro
On Thu, 2008-03-13 at 08:43 -0700, Daniel Walker wrote:
> On Wed, 2008-03-12 at 22:40 +0100, Jiri Kosina wrote:
>
> > No, as it is a real bug if you use mutexes in this way. What happens if
> > process that has called open() on your device (and has not closed it yet)
> > calls fork()?
> > Another breakage scenario -- what if the filedescriptor is sent through
> > unix socket to another process? etc.
>
> There's a number of places where a semaphore is used across system
> calls.
>
> for instance the usb skeleton,
> drivers/usb/usb-skeleton.c
>
> Several of the watchdog drivers,
> drivers/watchdog/s3c2410_wdt.c
>
> These need to be removed, but the usage is clearly not compatible with
> the mutex API ..
>
> If you convert them to atomic counts then you loose the sleeping aspect
> of the semaphore, which you'd have to add back somehow.
>
> The only API that seems straight forward is using complete's .. Then you
> get an atomic count and all the sleeping function calls you might want..
> (include/linux/completion.h) The problem with complete's is that you
> can't start them out at "1" or "completed" unless you actually run
> complete() once during initialization (that's kind of ugly) ..
A simple waitqueue would work.
On Thu, 2008-03-13 at 18:39 +0100, Peter Zijlstra wrote:
> > These need to be removed, but the usage is clearly not compatible with
> > the mutex API ..
> >
> > If you convert them to atomic counts then you loose the sleeping aspect
> > of the semaphore, which you'd have to add back somehow.
> >
> > The only API that seems straight forward is using complete's .. Then you
> > get an atomic count and all the sleeping function calls you might want..
> > (include/linux/completion.h) The problem with complete's is that you
> > can't start them out at "1" or "completed" unless you actually run
> > complete() once during initialization (that's kind of ugly) ..
>
> A simple waitqueue would work.
I was just mulling over what the most straight forward way would be.. I
thought about wait queues, but the API didn't seem as easy to use a
completes..
Daniel