Al found a potential problem in epoll_create(), where the
file->private_data member was set after fd_install(). This is obviously
wrong since another thread might do a close() on that fd# before we set
the file->private_data member. This goes over 2.6.13 and passes a few
basic tests I've done here.
Signed-off-by: Davide Libenzi <[email protected]>
- Davide
diff -Nru linux-2.6.13.vanilla/fs/eventpoll.c linux-2.6.13/fs/eventpoll.c
--- linux-2.6.13.vanilla/fs/eventpoll.c 2005-09-16 15:20:46.000000000 -0700
+++ linux-2.6.13/fs/eventpoll.c 2005-09-16 15:21:08.000000000 -0700
@@ -231,8 +231,9 @@
static void ep_poll_safewake_init(struct poll_safewake *psw);
static void ep_poll_safewake(struct poll_safewake *psw, wait_queue_head_t *wq);
-static int ep_getfd(int *efd, struct inode **einode, struct file **efile);
-static int ep_file_init(struct file *file);
+static int ep_getfd(int *efd, struct inode **einode, struct file **efile,
+ struct eventpoll *ep);
+static int ep_alloc(struct eventpoll **pep);
static void ep_free(struct eventpoll *ep);
static struct epitem *ep_find(struct eventpoll *ep, struct file *file, int fd);
static void ep_use_epitem(struct epitem *epi);
@@ -501,38 +502,37 @@
asmlinkage long sys_epoll_create(int size)
{
int error, fd;
+ struct eventpoll *ep;
struct inode *inode;
struct file *file;
DNPRINTK(3, (KERN_INFO "[%p] eventpoll: sys_epoll_create(%d)\n",
current, size));
- /* Sanity check on the size parameter */
+ /*
+ * Sanity check on the size parameter, and create the internal data
+ * structure ( "struct eventpoll" ).
+ */
error = -EINVAL;
- if (size <= 0)
+ if (size <= 0 || (error = ep_alloc(&ep)))
goto eexit_1;
/*
* Creates all the items needed to setup an eventpoll file. That is,
* a file structure, and inode and a free file descriptor.
*/
- error = ep_getfd(&fd, &inode, &file);
- if (error)
- goto eexit_1;
-
- /* Setup the file internal data structure ( "struct eventpoll" ) */
- error = ep_file_init(file);
+ error = ep_getfd(&fd, &inode, &file, ep);
if (error)
goto eexit_2;
-
DNPRINTK(3, (KERN_INFO "[%p] eventpoll: sys_epoll_create(%d) = %d\n",
current, size, fd));
return fd;
eexit_2:
- sys_close(fd);
+ ep_free(ep);
+ kfree(ep);
eexit_1:
DNPRINTK(3, (KERN_INFO "[%p] eventpoll: sys_epoll_create(%d) = %d\n",
current, size, error));
@@ -706,7 +706,8 @@
/*
* Creates the file descriptor to be used by the epoll interface.
*/
-static int ep_getfd(int *efd, struct inode **einode, struct file **efile)
+static int ep_getfd(int *efd, struct inode **einode, struct file **efile,
+ struct eventpoll *ep)
{
struct qstr this;
char name[32];
@@ -756,7 +757,7 @@
file->f_op = &eventpoll_fops;
file->f_mode = FMODE_READ;
file->f_version = 0;
- file->private_data = NULL;
+ file->private_data = ep;
/* Install the new setup file into the allocated fd. */
fd_install(fd, file);
@@ -777,7 +778,7 @@
}
-static int ep_file_init(struct file *file)
+static int ep_alloc(struct eventpoll **pep)
{
struct eventpoll *ep;
@@ -792,9 +793,9 @@
INIT_LIST_HEAD(&ep->rdllist);
ep->rbr = RB_ROOT;
- file->private_data = ep;
+ *pep = ep;
- DNPRINTK(3, (KERN_INFO "[%p] eventpoll: ep_file_init() ep=%p\n",
+ DNPRINTK(3, (KERN_INFO "[%p] eventpoll: ep_alloc() ep=%p\n",
current, ep));
return 0;
}
Davide Libenzi <[email protected]> wrote:
>
> diff -Nru linux-2.6.13.vanilla/fs/eventpoll.c linux-2.6.13/fs/eventpoll.c
> --- linux-2.6.13.vanilla/fs/eventpoll.c 2005-09-16 15:20:46.000000000 -0700
> +++ linux-2.6.13/fs/eventpoll.c 2005-09-16 15:21:08.000000000 -0700
> @@ -231,8 +231,9 @@
>
> static void ep_poll_safewake_init(struct poll_safewake *psw);
> static void ep_poll_safewake(struct poll_safewake *psw, wait_queue_head_t *wq);
> -static int ep_getfd(int *efd, struct inode **einode, struct file **efile);
> -static int ep_file_init(struct file *file);
> +static int ep_getfd(int *efd, struct inode **einode, struct file **efile,
> + struct eventpoll *ep);
> +static int ep_alloc(struct eventpoll **pep);
Sigh. Space-stuffing strikes again. Please resend as an attachment.
The number of whitespace-buggered patches which are coming in is just
getting out of control lately.
Even `patch -l' tossed four rejects, so there may be something else wrong
in this one.
Davide> Al found a potential problem in epoll_create(), where the
Davide> file->private_data member was set after fd_install(). This is
Davide> obviously wrong since another thread might do a close() on
Davide> that fd# before we set the file->private_data member. This
Davide> goes over 2.6.13 and passes a few basic tests I've done here.
Actually, I found the problem after Al pointed out a similar bug in my code ;)
- R.
On Fri, 16 Sep 2005, Andrew Morton wrote:
> Davide Libenzi <[email protected]> wrote:
>>
>> diff -Nru linux-2.6.13.vanilla/fs/eventpoll.c linux-2.6.13/fs/eventpoll.c
>> --- linux-2.6.13.vanilla/fs/eventpoll.c 2005-09-16 15:20:46.000000000 -0700
>> +++ linux-2.6.13/fs/eventpoll.c 2005-09-16 15:21:08.000000000 -0700
>> @@ -231,8 +231,9 @@
>>
>> static void ep_poll_safewake_init(struct poll_safewake *psw);
>> static void ep_poll_safewake(struct poll_safewake *psw, wait_queue_head_t *wq);
>> -static int ep_getfd(int *efd, struct inode **einode, struct file **efile);
>> -static int ep_file_init(struct file *file);
>> +static int ep_getfd(int *efd, struct inode **einode, struct file **efile,
>> + struct eventpoll *ep);
>> +static int ep_alloc(struct eventpoll **pep);
>
> Sigh. Space-stuffing strikes again. Please resend as an attachment.
>
> The number of whitespace-buggered patches which are coming in is just
> getting out of control lately.
>
> Even `patch -l' tossed four rejects, so there may be something else wrong
> in this one.
My side or your side? Anyway, see if the one attached merges cleanly ...
- Davide
Davide Libenzi <[email protected]> wrote:
>
> > Sigh. Space-stuffing strikes again. Please resend as an attachment.
> >
> > The number of whitespace-buggered patches which are coming in is just
> > getting out of control lately.
> >
> > Even `patch -l' tossed four rejects, so there may be something else wrong
> > in this one.
>
> My side or your side?
Not mine, pal ;)
Although sylpheed could perhaps do a better job of decrypting some of the
oddities which arrive.
> Anyway, see if the one attached merges cleanly ...
It does.
Andrew complained:
> The number of whitespace-buggered patches which are coming in is just
> getting out of control lately.
If people didn't use email clients, but rather a patch sending script,
it would work better. It is easier to send patch sets this way (you get
to edit all the vital fields, such as Subject, To and Cc lists, in your
favorite text editor), and the usual failures that mess up patch
sending are avoided.
The script I'm using is in good shape, and several others besides
myself are successfully using it to send patches to lkml.
See the embedded Usage string for documentation.
http://www.speakeasy.org/~pj99/sgi/sendpatchset
It handles sending one or several related patches, to a list of email
addresses. You prepare a text directive file with the addresses,
subjects and pathnames to the files containing the message contents.
Then you send it all off with a single invocation of this 'sendpatchset'
script.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401
On Fri, Sep 16, 2005 at 04:59:02PM -0700, Roland Dreier wrote:
> Davide> Al found a potential problem in epoll_create(), where the
> Davide> file->private_data member was set after fd_install(). This is
> Davide> obviously wrong since another thread might do a close() on
> Davide> that fd# before we set the file->private_data member. This
> Davide> goes over 2.6.13 and passes a few basic tests I've done here.
>
> Actually, I found the problem after Al pointed out a similar bug in my code ;)
Yup.