[PATCH 0/4] files: various updates

[PATCH 0/4] files: various updates

[Dipankar Sarma]

Andrew,

Here is the set of update patches that fixes, changes and documents
various issues in the new fd management patchset in -mm1. The
first patch - fix-dupfd-reacquire-fdt.patch fixes bugme #4770
and the oopses people other than me were seeing.

I have tested this patchset with 2.6.12-mm1 in my lab, not sure
if it means anything, given bugme #4770 and my userland :-)
Please include these in -mm1 for testing.

Thanks
Dipankar

Re: [PATCH 1/4] files: fix dupfd by fdt reload

[Dipankar Sarma]

locate_fd() may expand fdtable, so the fdtable pointer must be
reloaded after locate_fd(). Fixes bugme #4770.

Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com>
---


 fs/fcntl.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

diff -puN fs/fcntl.c~fix-dupfd-reacquire-fdt fs/fcntl.c
--- linux-2.6.12-mm1-fix/fs/fcntl.c~fix-dupfd-reacquire-fdt	2005-06-25 14:56:58.000000000 +0530
+++ linux-2.6.12-mm1-fix-dipankar/fs/fcntl.c	2005-06-25 14:58:26.000000000 +0530
@@ -118,9 +118,10 @@ static int dupfd(struct file *file, unsi
 	int fd;
 
 	spin_lock(&files->file_lock);
-	fdt = files_fdtable(files);
 	fd = locate_fd(files, file, start);
 	if (fd >= 0) {
+		/* locate_fd() may have expanded fdtable, load the ptr */
+		fdt = files_fdtable(files);
 		FD_SET(fd, fdt->open_fds);
 		FD_CLR(fd, fdt->close_on_exec);
 		spin_unlock(&files->file_lock);

_

Re: [PATCH 2/4] files: fix expand_files return code

[Dipankar Sarma]

If expand_fdtable() sees that someone else expanded the fdtable
while it dropped the lock, it can return 0 which in turn
can be returned by expand_files() even though there has
been an expansion of the fdtable since expand_files()
was originally called. This could lead to locate_fd()
not repeating the fd search and returning a bogus fd.
This patch fixes this problem.

Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com>
---


 fs/file.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff -puN fs/file.c~fix-expand-files fs/file.c
--- linux-2.6.12-mm1-fix/fs/file.c~fix-expand-files	2005-06-25 16:42:18.000000000 +0530
+++ linux-2.6.12-mm1-fix-dipankar/fs/file.c	2005-06-25 16:42:18.000000000 +0530
@@ -304,13 +304,14 @@ out:
 /*
  * Expands the file descriptor table - it will allocate a new fdtable and
  * both fd array and fdset. It is expected to be called with the
- * files_lock held.
+ * files_lock held. It returns 1 if fdtable expanded or -errno if
+ * expansion failed.
  */
 static int expand_fdtable(struct files_struct *files, int nr)
 	__releases(files->file_lock)
 	__acquires(files->file_lock)
 {
-	int error = 0;
+	int error = 1;
 	struct fdtable *fdt;
 	struct fdtable *nfdt = NULL;
 
@@ -350,7 +351,7 @@ out:
  */
 int expand_files(struct files_struct *files, int nr)
 {
-	int err, expand = 0;
+	int err;
 	struct fdtable *fdt;
 
 	fdt = files_fdtable(files);
@@ -360,11 +361,9 @@ int expand_files(struct files_struct *fi
 			err = -EMFILE;
 			goto out;
 		}
-		expand = 1;
-		if ((err = expand_fdtable(files, nr)))
-			goto out;
-	}
-	err = expand;
+		err = expand_fdtable(files, nr);
+	} else 
+		err = 0;
 out:
 	return err;
 }

_

Re: [PATCH 3/4] files: change fd_install assertion

[Dipankar Sarma]

Change the fds[fd] != NULL check in fd_install() to be a BUG_ON.

Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com>
---


 fs/open.c |    3 +--
 1 files changed, 1 insertion(+), 2 deletions(-)

diff -puN fs/open.c~change-fd-install-assert fs/open.c
--- linux-2.6.12-mm1-fix/fs/open.c~change-fd-install-assert	2005-06-25 16:43:02.000000000 +0530
+++ linux-2.6.12-mm1-fix-dipankar/fs/open.c	2005-06-25 16:43:02.000000000 +0530
@@ -931,8 +931,7 @@ void fastcall fd_install(unsigned int fd
 	struct fdtable *fdt;
 	spin_lock(&files->file_lock);
 	fdt = files_fdtable(files);
-	if (unlikely(fdt->fd[fd] != NULL))
-		BUG();
+	BUG_ON(fdt->fd[fd] != NULL);
 	rcu_assign_pointer(fdt->fd[fd], file);
 	spin_unlock(&files->file_lock);
 }

_

Re: [PATCH 4/4] files: doc update

[Dipankar Sarma]

Update files documentation to mention the need for reloading
fdtable pointer if ->file_lock is dropped.

Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com>
---


 Documentation/filesystems/files.txt |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+)

diff -puN Documentation/filesystems/files.txt~files-doc-update Documentation/filesystems/files.txt
--- linux-2.6.12-mm1-fix/Documentation/filesystems/files.txt~files-doc-update	2005-06-26 06:01:40.000000000 +0530
+++ linux-2.6.12-mm1-fix-dipankar/Documentation/filesystems/files.txt	2005-06-26 06:13:09.000000000 +0530
@@ -101,3 +101,23 @@ the fdtable structure -
    API. If they are looked up lock-free, rcu_dereference()
    must be used. However it is advisable to use files_fdtable()
    and fcheck()/fcheck_files() which take care of these issues.
+
+7. While updating, the fdtable pointer must be looked up while
+   holding files->file_lock. If ->file_lock is dropped, then
+   another thread expand the files thereby creating a new
+   fdtable and making the earlier fdtable pointer stale.
+   For example :
+
+	spin_lock(&files->file_lock);
+	fd = locate_fd(files, file, start);
+	if (fd >= 0) {
+		/* locate_fd() may have expanded fdtable, load the ptr */
+		fdt = files_fdtable(files);
+		FD_SET(fd, fdt->open_fds);
+		FD_CLR(fd, fdt->close_on_exec);
+		spin_unlock(&files->file_lock);
+	.....
+
+   Since locate_fd() can drop ->file_lock (and reacquire ->file_lock),
+   the fdtable pointer (fdt) must be loaded after locate_fd().
+

_

Re: [PATCH 3/4] files: change fd_install assertion

[Artem B. Bityuckiy]

Dipankar Sarma wrote:
> -	if (unlikely(fdt->fd[fd] != NULL))
> -		BUG();
> +	BUG_ON(fdt->fd[fd] != NULL);
>  	rcu_assign_pointer(fdt->fd[fd], file);
>  	spin_unlock(&files->file_lock);
>  }
> 
Why is this better ?


-- 
Best regards, Artem B. Bityuckiy
Oktet Labs (St. Petersburg), Software Engineer.
+78124286709 (office) +79112449030 (mobile)
E-mail: dedekind@oktetlabs.ru, web: http://www.oktetlabs.ru

Re: [PATCH 3/4] files: change fd_install assertion

[Dipankar Sarma]

On Fri, Jun 24, 2005 at 04:04:37PM +0400, Artem B. Bityuckiy wrote:
> Dipankar Sarma wrote:
> >-	if (unlikely(fdt->fd[fd] != NULL))
> >-		BUG();
> >+	BUG_ON(fdt->fd[fd] != NULL);
> > 	rcu_assign_pointer(fdt->fd[fd], file);
> > 	spin_unlock(&files->file_lock);
> > }
> >
> Why is this better ?

Because that way the compare and branch can be ifdefed out when CONFIG_BUG is
not set. Not to mention BUG_ON() looks more like an assertion.

Thanks
Dipankar

Re: [PATCH 3/4] files: change fd_install assertion

[Artem B. Bityuckiy]

Dipankar Sarma wrote:
> Because that way the compare and branch can be ifdefed out when CONFIG_BUG is
> not set. Not to mention BUG_ON() looks more like an assertion.

Surely, even if BUG() will be nothing, the compiler will optimize that?
Yes, it looks better, but I don't like that there was unlikely() before 
and you removed it. I'ts minor though.

-- 
Best regards, Artem B. Bityuckiy
Oktet Labs (St. Petersburg), Software Engineer.
+78124286709 (office) +79112449030 (mobile)
E-mail: dedekind@oktetlabs.ru, web: http://www.oktetlabs.ru

Re: [PATCH 3/4] files: change fd_install assertion

[Dipankar Sarma]

On Fri, Jun 24, 2005 at 05:55:21PM +0400, Artem B. Bityuckiy wrote:
> Dipankar Sarma wrote:
> >Because that way the compare and branch can be ifdefed out when CONFIG_BUG 
> >is
> >not set. Not to mention BUG_ON() looks more like an assertion.
> 
> Surely, even if BUG() will be nothing, the compiler will optimize that?
> Yes, it looks better, but I don't like that there was unlikely() before 
> and you removed it. I'ts minor though.

I did not. BUG_ON() is supposed to have unlikely() inside. See the
generic version. If an arch specific BUG_ON() doesn't have some
branch hint, it definitely should.

Thanks
Dipankar

Re: [PATCH 3/4] files: change fd_install assertion

[Artem B. Bityuckiy]

Dipankar Sarma wrote:
> I did not. BUG_ON() is supposed to have unlikely() inside. See the
> generic version. If an arch specific BUG_ON() doesn't have some
> branch hint, it definitely should.
Ah, pardon.

-- 
Best regards, Artem B. Bityuckiy
Oktet Labs (St. Petersburg), Software Engineer.
+78124286709 (office) +79112449030 (mobile)
E-mail: dedekind@oktetlabs.ru, web: http://www.oktetlabs.ru