[PATCH] fs: clear close-on-exec flag as part of put_unused_fd()

[PATCH] fs: clear close-on-exec flag as part of put_unused_fd()

[Yann Droneaud]

close-on-exec flag is set by get_unused_fd_flags(), it makes
sense to clear it in the opposite function, eg. put_unused_fd().

Additionally, since the close_on_exec bit array is always initialized
to 0, it can be safely assumed that any newly allocated file descriptor
has close-on-exec flag not set, so there's no need to clear it
explicitly.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 fs/file.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 4a78f981557a..e98f5a5b1050 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -500,8 +500,6 @@ repeat:
 	__set_open_fd(fd, fdt);
 	if (flags & O_CLOEXEC)
 		__set_close_on_exec(fd, fdt);
-	else
-		__clear_close_on_exec(fd, fdt);
 	error = fd;
 #if 1
 	/* Sanity check */
@@ -530,6 +528,7 @@ EXPORT_SYMBOL(get_unused_fd_flags);
 static void __put_unused_fd(struct files_struct *files, unsigned int fd)
 {
 	struct fdtable *fdt = files_fdtable(files);
+	__clear_close_on_exec(fd, fdt);
 	__clear_open_fd(fd, fdt);
 	if (fd < files->next_fd)
 		files->next_fd = fd;
@@ -599,7 +598,6 @@ int __close_fd(struct files_struct *files, unsigned fd)
 	if (!file)
 		goto out_unlock;
 	rcu_assign_pointer(fdt->fd[fd], NULL);
-	__clear_close_on_exec(fd, fdt);
 	__put_unused_fd(files, fd);
 	spin_unlock(&files->file_lock);
 	return filp_close(file, files);
@@ -625,7 +623,6 @@ void do_close_on_exec(struct files_struct *files)
 		set = fdt->close_on_exec[i];
 		if (!set)
 			continue;
-		fdt->close_on_exec[i] = 0;
 		for ( ; set ; fd++, set >>= 1) {
 			struct file *file;
 			if (!(set & 1))
@@ -806,8 +803,6 @@ static int do_dup2(struct files_struct *files,
 	__set_open_fd(fd, fdt);
 	if (flags & O_CLOEXEC)
 		__set_close_on_exec(fd, fdt);
-	else
-		__clear_close_on_exec(fd, fdt);
 	spin_unlock(&files->file_lock);
 
 	if (tofree)
-- 
1.8.4.2

Re: [PATCH] fs: clear close-on-exec flag as part of put_unused_fd()

[Mateusz Guzik]

On Wed, Dec 11, 2013 at 10:08:27PM +0100, Yann Droneaud wrote:
> close-on-exec flag is set by get_unused_fd_flags(), it makes
> sense to clear it in the opposite function, eg. put_unused_fd().
> 
> Additionally, since the close_on_exec bit array is always initialized
> to 0, it can be safely assumed that any newly allocated file descriptor
> has close-on-exec flag not set, so there's no need to clear it
> explicitly.
> 
> Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
> ---
>  fs/file.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 4a78f981557a..e98f5a5b1050 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -500,8 +500,6 @@ repeat:
>  	__set_open_fd(fd, fdt);
>  	if (flags & O_CLOEXEC)
>  		__set_close_on_exec(fd, fdt);
> -	else
> -		__clear_close_on_exec(fd, fdt);
>  	error = fd;
>  #if 1
>  	/* Sanity check */
> @@ -530,6 +528,7 @@ EXPORT_SYMBOL(get_unused_fd_flags);
>  static void __put_unused_fd(struct files_struct *files, unsigned int fd)
>  {
>  	struct fdtable *fdt = files_fdtable(files);
> +	__clear_close_on_exec(fd, fdt);
>  	__clear_open_fd(fd, fdt);
>  	if (fd < files->next_fd)
>  		files->next_fd = fd;
> @@ -599,7 +598,6 @@ int __close_fd(struct files_struct *files, unsigned fd)
>  	if (!file)
>  		goto out_unlock;
>  	rcu_assign_pointer(fdt->fd[fd], NULL);
> -	__clear_close_on_exec(fd, fdt);
>  	__put_unused_fd(files, fd);
>  	spin_unlock(&files->file_lock);
>  	return filp_close(file, files);
> @@ -625,7 +623,6 @@ void do_close_on_exec(struct files_struct *files)
>  		set = fdt->close_on_exec[i];
>  		if (!set)
>  			continue;
> -		fdt->close_on_exec[i] = 0;
>  		for ( ; set ; fd++, set >>= 1) {
>  			struct file *file;
>  			if (!(set & 1))
> @@ -806,8 +803,6 @@ static int do_dup2(struct files_struct *files,
>  	__set_open_fd(fd, fdt);
>  	if (flags & O_CLOEXEC)
>  		__set_close_on_exec(fd, fdt);
> -	else
> -		__clear_close_on_exec(fd, fdt);
>  	spin_unlock(&files->file_lock);
>  
>  	if (tofree)

>From my reading this will break at least the following:
fd = open(..., .. | O_CLOEXEC);
dup2(whatever, fd);

now fd has O_CLOEXEC even though it should not

-- 
Mateusz Guzik

Re: [PATCH] fs: clear close-on-exec flag as part of put_unused_fd()

[Al Viro]

On Wed, Dec 11, 2013 at 11:36:35PM +0100, Mateusz Guzik wrote:

> >From my reading this will break at least the following:
> fd = open(..., .. | O_CLOEXEC);
> dup2(whatever, fd);
> 
> now fd has O_CLOEXEC even though it should not

Moreover, consider fork() done by a thread that shares descriptor
table with somebody else.  Suppose it happens in the middle of
open() with O_CLOEXEC being done by another thread.  We copy descriptor
table after descriptor had been reserved (and marked close-on-exec),
but before a reference to struct file has actually been inserted there.
This code
        for (i = open_files; i != 0; i--) {
                struct file *f = *old_fds++;
                if (f) {
                        get_file(f);
                } else {
                        /*    
                         * The fd may be claimed in the fd bitmap but not yet
                         * instantiated in the files array if a sibling thread
                         * is partway through open().  So make sure that this
                         * fd is available to the new process.
                         */
                        __clear_open_fd(open_files - i, new_fdt);
                }
                rcu_assign_pointer(*new_fds++, f);
        }
        spin_unlock(&oldf->file_lock);
in dup_fd() will clear the corresponding bit in open_fds, leaving close_on_exec
alone.  Currently that's fine (we will override whatever had been in
close_on_exec when we reserve that descriptor again), but AFAICS with this
patch it will break.

Sure, it can be fixed up (ditto with dup2(), etc.), but what's the point?
Result will require more subtle reasoning to prove correctness and will
be more prone to breakage.  Does that really yield visible performance
improvements that would be worth the extra complexity?  After all, you
trade some writes to close_on_exec on descriptor reservation for unconditional
write on descriptor freeing; if anything, I would expect that you'll get
minor _loss_ from that change, assuming they'll be measurable in the first
place...

Re: [PATCH] fs: clear close-on-exec flag as part of put_unused_fd()

[Yann Droneaud]

Hi,

Le mercredi 11 décembre 2013 à 23:36 +0100, Mateusz Guzik a écrit :
> On Wed, Dec 11, 2013 at 10:08:27PM +0100, Yann Droneaud wrote:
> > @@ -806,8 +803,6 @@ static int do_dup2(struct files_struct *files,
> >  	__set_open_fd(fd, fdt);
> >  	if (flags & O_CLOEXEC)
> >  		__set_close_on_exec(fd, fdt);
> > -	else
> > -		__clear_close_on_exec(fd, fdt);
> >  	spin_unlock(&files->file_lock);
> >  
> >  	if (tofree)
> 
> From my reading this will break at least the following:
> fd = open(..., .. | O_CLOEXEC);
> dup2(whatever, fd);
> 
> now fd has O_CLOEXEC even though it should not
> 

Thanks for the review.

You're right.

I've misunderstood the portion of the code handling the case where the
destination fd is already marked as used: -EBUSY is only returned when
the file descriptor is allocaged but not yet installed.

So close-on-exec flag must be cleared in do_dup2().

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs: clear close-on-exec flag as part of put_unused_fd()

[Yann Droneaud]

Hi,

Le mercredi 11 décembre 2013 à 23:30 +0000, Al Viro a écrit :
> On Wed, Dec 11, 2013 at 11:36:35PM +0100, Mateusz Guzik wrote:
> 
> > >From my reading this will break at least the following:
> > fd = open(..., .. | O_CLOEXEC);
> > dup2(whatever, fd);
> > 
> > now fd has O_CLOEXEC even though it should not
> 
> Moreover, consider fork() done by a thread that shares descriptor
> table with somebody else.  Suppose it happens in the middle of
> open() with O_CLOEXEC being done by another thread.  We copy descriptor
> table after descriptor had been reserved (and marked close-on-exec),
> but before a reference to struct file has actually been inserted there.
> This code
>         for (i = open_files; i != 0; i--) {
>                 struct file *f = *old_fds++;
>                 if (f) {
>                         get_file(f);
>                 } else {
>                         /*    
>                          * The fd may be claimed in the fd bitmap but not yet
>                          * instantiated in the files array if a sibling thread
>                          * is partway through open().  So make sure that this
>                          * fd is available to the new process.
>                          */
>                         __clear_open_fd(open_files - i, new_fdt);
>                 }
>                 rcu_assign_pointer(*new_fds++, f);
>         }
>         spin_unlock(&oldf->file_lock);
> in dup_fd() will clear the corresponding bit in open_fds, leaving close_on_exec
> alone.  Currently that's fine (we will override whatever had been in
> close_on_exec when we reserve that descriptor again), but AFAICS with this
> patch it will break.
> 

That's a terrible subtle case. it will indeed break with the patch.

> Sure, it can be fixed up (ditto with dup2(), etc.), but what's the point?

It was only an attempt at making close-on-exec handling "simpler".

> Result will require more subtle reasoning to prove correctness and will
> be more prone to breakage.  Does that really yield visible performance
> improvements that would be worth the extra complexity?  After all, you
> trade some writes to close_on_exec on descriptor reservation for unconditional
> write on descriptor freeing; if anything, I would expect that you'll get
> minor _loss_ from that change, assuming they'll be measurable in the first
> place...

Since it's not so straightforward to get it correct, and the only
advantage I was trying to address is aesthetic, I will discard it.

Thanks a lot for the review and the comments.

Regards.

-- 
Yann Droneaud
OPTEYA

[PATCH] fs: bits in .close_on_exec are only defined for matching bits in .open_fds bits

[Yann Droneaud]

Flag close-on-exec can only be set on an allocated (but perhaps not yet
installed) file descriptor. So if the bit in struct fdtable .open_fds array
is not set, then value of matching bit in the .close_on_exec array is
meaningless.

This patch rely on this property to
- remove initialization of unused part of .close_on_exec array;
- remove clear of .close_on_exec bit when releasing a file descriptor.
The patch takes care of adding the required check on .open_fds bit
before looking for .close_on_exec bit.

Link: http://lkml.kernel.org/r/1386796107-4197-1-git-send-email-ydroneaud@opteya.com
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---

Hi Al and Mateusz,

First of all, thank you for reviewing my previous patch and pointing out
the error I've missed.

Please consider this new patch which take the opposite approach:
my previous patch assumed that .close_on_exec bit where defaulting to 0,
but you prove this was a wrong assumption. This new patch assume that
.close_on_exec bit are in a unknown, meaningless value when the file
descriptor is not allocated. This way, there's no need to clear the value
when releasing a file descriptor, and there's no need to initialize the
.close_on_exec array.

Unlike my previous patch, I haven't yet tested it. It's known to compile.

Please try to find some corner cases I've missed in this other attempt.

Regards.


 fs/file.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 4a78f981557a..3016e09d0290 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -78,7 +78,7 @@ static void copy_fdtable(struct fdtable *nfdt, struct fdtable *ofdt)
 	memcpy(nfdt->open_fds, ofdt->open_fds, cpy);
 	memset((char *)(nfdt->open_fds) + cpy, 0, set);
 	memcpy(nfdt->close_on_exec, ofdt->close_on_exec, cpy);
-	memset((char *)(nfdt->close_on_exec) + cpy, 0, set);
+	/* remaining portion of close_on_exec left uninitialized */
 }
 
 static struct fdtable * alloc_fdtable(unsigned int nr)
@@ -335,7 +335,7 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
 		int start = open_files / BITS_PER_LONG;
 
 		memset(&new_fdt->open_fds[start], 0, left);
-		memset(&new_fdt->close_on_exec[start], 0, left);
+		/* remaining portion of close_on_exec left uninitialized */
 	}
 
 	rcu_assign_pointer(newf->fdt, new_fdt);
@@ -599,7 +599,6 @@ int __close_fd(struct files_struct *files, unsigned fd)
 	if (!file)
 		goto out_unlock;
 	rcu_assign_pointer(fdt->fd[fd], NULL);
-	__clear_close_on_exec(fd, fdt);
 	__put_unused_fd(files, fd);
 	spin_unlock(&files->file_lock);
 	return filp_close(file, files);
@@ -622,10 +621,9 @@ void do_close_on_exec(struct files_struct *files)
 		fdt = files_fdtable(files);
 		if (fd >= fdt->max_fds)
 			break;
-		set = fdt->close_on_exec[i];
+		set = fdt->close_on_exec[i] & fdt->open_fds[i];
 		if (!set)
 			continue;
-		fdt->close_on_exec[i] = 0;
 		for ( ; set ; fd++, set >>= 1) {
 			struct file *file;
 			if (!(set & 1))
@@ -772,7 +770,7 @@ bool get_close_on_exec(unsigned int fd)
 	bool res;
 	rcu_read_lock();
 	fdt = files_fdtable(files);
-	res = close_on_exec(fd, fdt);
+	res = fd_is_open(fd, fdt) && close_on_exec(fd, fdt);
 	rcu_read_unlock();
 	return res;
 }
-- 
1.8.4.2