[patch] Fix epoll delayed initialization bug ...

[patch] Fix epoll delayed initialization bug ...

[Davide Libenzi]

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 <davidel@xmailserver.org>


- 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;
  }

Re: [patch] Fix epoll delayed initialization bug ...

[Andrew Morton]

Davide Libenzi <davidel@xmailserver.org> 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.

Re: [patch] Fix epoll delayed initialization bug ...

[Davide Libenzi]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1157 bytes --]

On Fri, 16 Sep 2005, Andrew Morton wrote:

> Davide Libenzi <davidel@xmailserver.org> 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

[-- Attachment #2: Type: TEXT/plain, Size: 3118 bytes --]

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)) != 0)
 		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;
 }

Re: [patch] Fix epoll delayed initialization bug ...

[Andrew Morton]

Davide Libenzi <davidel@xmailserver.org> 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.

Re: [patch] Fix epoll delayed initialization bug ...

[Paul Jackson]

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 <pj@sgi.com> 1.925.600.0401

Re: [patch] Fix epoll delayed initialization bug ...

[Roland Dreier]

    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.

Re: [patch] Fix epoll delayed initialization bug ...

[Al Viro]

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.