2007-05-07 22:20:14

by Tim Hockin

[permalink] [raw]
Subject: [PATCH] x86_64: O_EXCL on /dev/mcelog (resend)

From: Tim Hockin <[email protected]>

Background:
/dev/mcelog is a clear-on-read interface. It is currently possible for
multiple users to open and read() the device. Users are protected from
each other during any one read, but not across reads.

Description:
This patch adds support for O_EXCL to /dev/mcelog. If a user opens the
device with O_EXCL, no other user may open the device (EBUSY). Likewise,
any user that tries to open the device with O_EXCL while another user has
the device will fail (EBUSY).

Result:
Applications can get exclusive access to /dev/mcelog. Applications that
do not care will be unchanged.

Alternatives:
A simpler choice would be to only allow one open() at all, regardless of
O_EXCL.

Testing:
I wrote an application that opens /dev/mcelog with O_EXCL and observed
that any other app that tried to open /dev/mcelog would fail until the
exclusive app had closed the device.

Caveats:
None.

Patch:
This patch is against 2.6.21.

Signed-off-by: Tim Hockin <[email protected]>

---

This is the first version version of this patch. The simpler alternative
of only one open() sounds better to me, but becomes a net change in
behavior.


diff -pruN linux-2.6.20+th/arch/x86_64/kernel/mce.c linux-2.6.20+th1.5/arch/x86_64/kernel/mce.c
--- linux-2.6.20+th/arch/x86_64/kernel/mce.c 2007-04-27 14:19:08.000000000 -0700
+++ linux-2.6.20+th1.5/arch/x86_64/kernel/mce.c 2007-05-01 21:53:10.000000000 -0700
@@ -465,6 +465,40 @@ void __cpuinit mcheck_init(struct cpuinf
* Character device to read and clear the MCE log.
*/

+static DEFINE_SPINLOCK(mce_state_lock);
+static int open_count; /* #times opened */
+static int open_exclu; /* already open exclusive? */
+
+static int mce_open(struct inode *inode, struct file *file)
+{
+ spin_lock(&mce_state_lock);
+
+ if (open_exclu || (open_count && (file->f_flags & O_EXCL))) {
+ spin_unlock(&mce_state_lock);
+ return -EBUSY;
+ }
+
+ if (file->f_flags & O_EXCL)
+ open_exclu = 1;
+ open_count++;
+
+ spin_unlock(&mce_state_lock);
+
+ return 0;
+}
+
+static int mce_release(struct inode *inode, struct file *file)
+{
+ spin_lock(&mce_state_lock);
+
+ open_count--;
+ open_exclu = 0;
+
+ spin_unlock(&mce_state_lock);
+
+ return 0;
+}
+
static void collect_tscs(void *data)
{
unsigned long *cpu_tsc = (unsigned long *)data;
@@ -553,6 +587,8 @@ static int mce_ioctl(struct inode *i, st
}

static const struct file_operations mce_chrdev_ops = {
+ .open = mce_open,
+ .release = mce_release,
.read = mce_read,
.ioctl = mce_ioctl,
};


2007-05-09 23:46:42

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] x86_64: O_EXCL on /dev/mcelog (resend)

On Mon, 7 May 2007 15:19:01 -0700
[email protected] (Tim Hockin) wrote:

> From: Tim Hockin <[email protected]>
>
> Background:
> /dev/mcelog is a clear-on-read interface. It is currently possible for
> multiple users to open and read() the device. Users are protected from
> each other during any one read, but not across reads.
>
> Description:
> This patch adds support for O_EXCL to /dev/mcelog. If a user opens the
> device with O_EXCL, no other user may open the device (EBUSY). Likewise,
> any user that tries to open the device with O_EXCL while another user has
> the device will fail (EBUSY).
>
> Result:
> Applications can get exclusive access to /dev/mcelog. Applications that
> do not care will be unchanged.
>
> Alternatives:
> A simpler choice would be to only allow one open() at all, regardless of
> O_EXCL.
>
> Testing:
> I wrote an application that opens /dev/mcelog with O_EXCL and observed
> that any other app that tried to open /dev/mcelog would fail until the
> exclusive app had closed the device.
>
> Caveats:
> None.
>
> Patch:
> This patch is against 2.6.21.
>
> Signed-off-by: Tim Hockin <[email protected]>
>
> ---
>
> This is the first version version of this patch. The simpler alternative
> of only one open() sounds better to me, but becomes a net change in
> behavior.
>
>
> diff -pruN linux-2.6.20+th/arch/x86_64/kernel/mce.c linux-2.6.20+th1.5/arch/x86_64/kernel/mce.c
> --- linux-2.6.20+th/arch/x86_64/kernel/mce.c 2007-04-27 14:19:08.000000000 -0700
> +++ linux-2.6.20+th1.5/arch/x86_64/kernel/mce.c 2007-05-01 21:53:10.000000000 -0700
> @@ -465,6 +465,40 @@ void __cpuinit mcheck_init(struct cpuinf
> * Character device to read and clear the MCE log.
> */
>
> +static DEFINE_SPINLOCK(mce_state_lock);
> +static int open_count; /* #times opened */
> +static int open_exclu; /* already open exclusive? */

"open_exclusive" ;)

> +static int mce_open(struct inode *inode, struct file *file)
> +{
> + spin_lock(&mce_state_lock);
> +
> + if (open_exclu || (open_count && (file->f_flags & O_EXCL))) {
> + spin_unlock(&mce_state_lock);
> + return -EBUSY;
> + }
> +
> + if (file->f_flags & O_EXCL)
> + open_exclu = 1;
> + open_count++;
> +
> + spin_unlock(&mce_state_lock);
> +
> + return 0;
> +}

Does this guy support lseek? If not we should toss a nonseekable_open()
call in here.

> +static int mce_release(struct inode *inode, struct file *file)
> +{
> + spin_lock(&mce_state_lock);
> +
> + open_count--;
> + open_exclu = 0;
> +
> + spin_unlock(&mce_state_lock);
> +
> + return 0;
> +}