Date:	Fri, 4 Jun 2010 01:47:26 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	Frederic Weisbecker <fweisbec@...il.com>,
	John Kacur <jkacur@...hat.com>, Ingo Molnar <mingo@...e.hu>,
	Jan Blunck <jblunck@...e.de>, Julia Lawall <julia@...u.dk>
Subject: Re: [RFC 4/5] BKL: use no BKL in llseek

On Thursday 03 June 2010 09:06:08 Christoph Hellwig wrote:
> On Thu, Jun 03, 2010 at 02:13:18AM +0200, Arnd Bergmann wrote:
> > We have shown that the BKL in default_llseek and other
> > llseek operations never protects against concurrent access
> > from another function:
> 
> From VFS POV no to the default_llseek conversion.  As told you and
> others of the BKL brigade don't change this thing but get rid of it
> entirely.  I was under the impression we had agreed on a road map
> for that anyway.  Please don't forget that overall kernel improvement
> should at least be a side effect of your big sweap and clean.

Well, the two things (killing the BKL and killing default_llseek)
are not as related as make it appear: as I've shown, nothing that
currently implicitly uses default_llseek depends on the BKL,
so we could kill them in either order.

Nevertheless, you are right that default_llseek needs to go as well,
and I've put some effort into this. This is a coccinelle patch
that tries to find the correct llseek method for each file_operation
(default_llseek, no_llseek, noop_llseek). It does not find the
correct answer in each case, and is likely much more complicated
than it needs to be.

Julia, can you take a look? This currently outputs more than
one .llseek= line for some operations and fails to make detect
some cases that I think it should find.

	Arnd

---
@ open1 @
identifier nested_open;
identifier nso ~= "nonseekable_open";
@@
nested_open(...)
{
...
nso(...)
...
}

@ open @
identifier open_f;
identifier i, f;
identifier nso ~= "nonseekable_open";
identifier open1.nested_open;
@@
int open_f(struct inode *i, struct file *f)
{
...
(
nso(...)
|
nested_open(...)
)
...
}

@ read @
identifier read_f;
identifier f, p, s, off;
type ssize_t, size_t, loff_t;
expression E;
identifier func;
@@
ssize_t read_f(struct file *f, char *p, size_t s, loff_t *off)
{
(
... *off = E...
|
... func(..., off, ...) ...
|
... E = *off ...
)
}

@ read_no_fpos @
identifier read_f;
identifier f, p, s, off;
type ssize_t, size_t, loff_t;
@@
ssize_t read_f(struct file *f, char *p, size_t s, loff_t *off)
{
... when != off
}

@ write @
identifier write_f;
identifier f, p, s, off;
type ssize_t, size_t, loff_t;
expression E;
identifier func;
@@
ssize_t write_f(struct file *f, const char *p, size_t s, loff_t *off)
{
... *off = E...
|
... func(..., off, ...) ...
|
... E = *off ...
)
}

@ write_no_fpos @
identifier write_f;
identifier f, p, s, off;
type ssize_t, size_t, loff_t;
@@
ssize_t write_f(struct file *f, const char *p, size_t s, loff_t *off)
{
... when != off
}

@ fops0 @
identifier fops;
@@
struct file_operations fops = {
 ...
};

@ has_llseek depends on fops0 @
identifier fops0.fops;
identifier llseek_f;
@@
struct file_operations fops = {
... 
 .llseek = llseek_f,
...
};

@ has_read depends on fops0 @
identifier fops0.fops;
identifier read_f;
@@
struct file_operations fops = {
... 
 .read = read_f,
...
};

@ has_write depends on fops0 @
identifier fops0.fops;
identifier write_f;
@@
struct file_operations fops = {
... 
 .write = write_f,
...
};

@ has_open depends on fops0 @
identifier fops0.fops;
identifier open_f;
@@
struct file_operations fops = {
... 
 .open = open_f,
...
};

// does not work properly -- this always matches...
@ nonseekable1 depends on !has_llseek && has_open @
identifier fops;
identifier nso ~= "nonseekable_open";
@@
// fops directly use nonseekable_open
struct file_operations fops = {
...  .open = nso, ...
+.llseek = no_llseek, /* nonseekable */
};

@ nonseekable2 depends on !has_llseek @
identifier fops;
identifier open.open_f;
@@
// fops use open which calls nonseekable_open
struct file_operations fops = {
...  .open = open_f, ...
+.llseek = no_llseek, /* open uses nonseekable */
};

@ fops1 depends on !has_llseek && !nonseekable2 @
identifier fops0.fops;
identifier read.read_f;
identifier readdir_e;
@@
(
// read fops use offset
struct file_operations fops = {
... .read = read_f, ...
+.llseek = default_llseek, /* read accesses f_pos */
};
|
// any other fop is used that changes pos
struct file_operations fops = {
... .readdir = readdir_e, ...
+.llseek = default_llseek, /* readdir is present */
};
)

@ fops2 depends on !fops1 && !has_llseek && !nonseekable2 @
identifier fops0.fops;
identifier write.write_f;
@@
// write fops use offset
struct file_operations fops = {
... .write = write_f, ...
+	.llseek = default_llseek, /* write accesses f_pos */
};

@ fops3 depends on !fops1 && !fops2 && !has_llseek && !nonseekable2 @
identifier fops;
identifier read_no_fpos.read_f;
identifier write_no_fpos.write_f;
@@
// write fops use offset
struct file_operations fops = {
... 
 .write = write_f,
 .read = read_f,
...
+.llseek = noop_llseek, /* read and write both use no f_pos */
};

@ depends on has_write && !has_read && !fops1 && !fops2 && !has_llseek && nonseekable2 @
identifier fops0.fops;
identifier write_no_fpos.write_f;
@@
struct file_operations fops = {
... .write = write_f, ...
+.llseek = noop_llseek, /* write uses no f_pos */
};

@ depends on has_read && !has_write && !fops1 && !fops2 && !has_llseek && !nonseekable2 @
identifier fops0.fops;
identifier read_no_fpos.read_f;
@@
struct file_operations fops = {
... .read = read_f, ...
+.llseek = noop_llseek, /* read uses no f_pos */
};

@ depends on !has_read && !has_write && !fops1 && !fops2 && !has_llseek && !nonseekable2@
identifier fops0.fops;
@@
struct file_operations fops = {
...
+.llseek = noop_llseek, /* no read or write fn */
};
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists