Linus, Andrew,
please revert 1ad3dcc0. That was a patch to the binfmt_flat loader,
which was motivated by an LTP testcase which checks that execve returns
EMFILE when the file descriptor table is full.
The patch is buggy: the code now keeps file descriptors open for the
executable and its libraries, which has confused at least one
application. It's also unnecessary, since there is no code that uses
the file descriptor, so the new EMFILE error return is totally artificial.
The reversion is
Signed-off-by: Bernd Schmidt <[email protected]>
Signed-off-by: Greg Ungerer <[email protected]>
and I think Luke had no objections either.
Bernd
Bernd Schmidt <[email protected]> wrote:
>
> Linus, Andrew,
>
> please revert 1ad3dcc0. That was a patch to the binfmt_flat loader,
> which was motivated by an LTP testcase which checks that execve returns
> EMFILE when the file descriptor table is full.
>
> The patch is buggy: the code now keeps file descriptors open for the
> executable and its libraries, which has confused at least one
> application. It's also unnecessary, since there is no code that uses
> the file descriptor, so the new EMFILE error return is totally artificial.
>
> The reversion is
> Signed-off-by: Bernd Schmidt <[email protected]>
> Signed-off-by: Greg Ungerer <[email protected]>
> and I think Luke had no objections either.
>
I don't get it. The substance of the patch is
+ /* check file descriptor */
+ exec_fileno = get_unused_fd();
+ if (exec_fileno < 0) {
+ ret = -EMFILE;
+ goto err;
+ }
+ get_file(bprm->file);
+ fd_install(exec_fileno, bprm->file);
and that get_file() will be undone by exit(). Without this change we'll
forget to do file limit checking.
So.. please tell us much more about the problem.
Andrew Morton wrote:
> Bernd Schmidt <[email protected]> wrote:
>> please revert 1ad3dcc0. That was a patch to the binfmt_flat loader,
>> which was motivated by an LTP testcase which checks that execve returns
>> EMFILE when the file descriptor table is full.
>>
>> The patch is buggy: the code now keeps file descriptors open for the
>> executable and its libraries, which has confused at least one
>> application. It's also unnecessary, since there is no code that uses
>> the file descriptor, so the new EMFILE error return is totally artificial.
> I don't get it. The substance of the patch is
>
> + /* check file descriptor */
> + exec_fileno = get_unused_fd();
> + if (exec_fileno < 0) {
> + ret = -EMFILE;
> + goto err;
> + }
> + get_file(bprm->file);
> + fd_install(exec_fileno, bprm->file);
>
> and that get_file() will be undone by exit(). Without this change we'll
> forget to do file limit checking.
It's not the get_file that's the problem, it's the get_unused_fd and
fd_install. These files are now open while the process lives and
consume file descriptors. This does not happen with the ELF loader,
which does
if (interpreter_type != INTERPRETER_AOUT)
sys_close(elf_exec_fileno);
before transferring control to the application. So, fewer file
descriptors are available for the app, and they start at a higher number.
Before the change, we didn't allocate or install a file descriptor,
hence there wasn't any reason to return EMFILE. The spec at
http://www.opengroup.org/onlinepubs/009695399/functions/exec.html
doesn't list EMFILE as a possible error.
If you're unconvinced, then at the very least we need to add a sys_close
call in the success path.
Bernd
Bernd Schmidt <[email protected]> wrote:
>
> Andrew Morton wrote:
> > Bernd Schmidt <[email protected]> wrote:
> >> please revert 1ad3dcc0. That was a patch to the binfmt_flat loader,
> >> which was motivated by an LTP testcase which checks that execve returns
> >> EMFILE when the file descriptor table is full.
> >>
> >> The patch is buggy: the code now keeps file descriptors open for the
> >> executable and its libraries, which has confused at least one
> >> application. It's also unnecessary, since there is no code that uses
> >> the file descriptor, so the new EMFILE error return is totally artificial.
>
> > I don't get it. The substance of the patch is
> >
> > + /* check file descriptor */
> > + exec_fileno = get_unused_fd();
> > + if (exec_fileno < 0) {
> > + ret = -EMFILE;
> > + goto err;
> > + }
> > + get_file(bprm->file);
> > + fd_install(exec_fileno, bprm->file);
> >
> > and that get_file() will be undone by exit(). Without this change we'll
> > forget to do file limit checking.
>
> It's not the get_file that's the problem, it's the get_unused_fd and
> fd_install. These files are now open while the process lives and
> consume file descriptors. This does not happen with the ELF loader,
> which does
>
> if (interpreter_type != INTERPRETER_AOUT)
> sys_close(elf_exec_fileno);
>
> before transferring control to the application. So, fewer file
> descriptors are available for the app, and they start at a higher number.
OIC.
> Before the change, we didn't allocate or install a file descriptor,
> hence there wasn't any reason to return EMFILE. The spec at
> http://www.opengroup.org/onlinepubs/009695399/functions/exec.html
> doesn't list EMFILE as a possible error.
>
> If you're unconvinced, then at the very least we need to add a sys_close
> call in the success path.
>
If we do that, we lose the ability to fail the exec if the fd table is
full. So we permit applications to temporarily exceed their limit by one
fd. Big deal.
And yes, the fact that the kernel internally and temporarily uses a file*
is an implementation detail which userspace doesn't need to care about
(yes?). In which case LTP is being silly.
It'd be nice not to lose the coding cleanups which that patch needed. Can
you review-n-test this form of reversion?
diff -puN fs/binfmt_flat.c~binfmt_flat-dont-check-for-emfile fs/binfmt_flat.c
--- devel/fs/binfmt_flat.c~binfmt_flat-dont-check-for-emfile 2006-05-16 07:42:55.000000000 -0700
+++ devel-akpm/fs/binfmt_flat.c 2006-05-16 07:44:07.000000000 -0700
@@ -428,7 +428,6 @@ static int load_flat_file(struct linux_b
loff_t fpos;
unsigned long start_code, end_code;
int ret;
- int exec_fileno;
hdr = ((struct flat_hdr *) bprm->buf); /* exec-header */
inode = bprm->file->f_dentry->d_inode;
@@ -502,21 +501,12 @@ static int load_flat_file(struct linux_b
goto err;
}
- /* check file descriptor */
- exec_fileno = get_unused_fd();
- if (exec_fileno < 0) {
- ret = -EMFILE;
- goto err;
- }
- get_file(bprm->file);
- fd_install(exec_fileno, bprm->file);
-
/* Flush all traces of the currently running executable */
if (id == 0) {
result = flush_old_exec(bprm);
if (result) {
ret = result;
- goto err_close;
+ goto err;
}
/* OK, This is the point of no return */
@@ -548,7 +538,7 @@ static int load_flat_file(struct linux_b
textpos = (unsigned long) -ENOMEM;
printk("Unable to mmap process text, errno %d\n", (int)-textpos);
ret = textpos;
- goto err_close;
+ goto err;
}
down_write(¤t->mm->mmap_sem);
@@ -564,7 +554,7 @@ static int load_flat_file(struct linux_b
(int)-datapos);
do_munmap(current->mm, textpos, text_len);
ret = realdatastart;
- goto err_close;
+ goto err;
}
datapos = realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long);
@@ -587,7 +577,7 @@ static int load_flat_file(struct linux_b
do_munmap(current->mm, textpos, text_len);
do_munmap(current->mm, realdatastart, data_len + extra);
ret = result;
- goto err_close;
+ goto err;
}
reloc = (unsigned long *) (datapos+(ntohl(hdr->reloc_start)-text_len));
@@ -606,7 +596,7 @@ static int load_flat_file(struct linux_b
printk("Unable to allocate RAM for process text/data, errno %d\n",
(int)-textpos);
ret = textpos;
- goto err_close;
+ goto err;
}
realdatastart = textpos + ntohl(hdr->data_start);
@@ -652,7 +642,7 @@ static int load_flat_file(struct linux_b
do_munmap(current->mm, textpos, text_len + data_len + extra +
MAX_SHARED_LIBS * sizeof(unsigned long));
ret = result;
- goto err_close;
+ goto err;
}
}
@@ -717,7 +707,7 @@ static int load_flat_file(struct linux_b
addr = calc_reloc(*rp, libinfo, id, 0);
if (addr == RELOC_FAILED) {
ret = -ENOEXEC;
- goto err_close;
+ goto err;
}
*rp = addr;
}
@@ -747,7 +737,7 @@ static int load_flat_file(struct linux_b
rp = (unsigned long *) calc_reloc(addr, libinfo, id, 1);
if (rp == (unsigned long *)RELOC_FAILED) {
ret = -ENOEXEC;
- goto err_close;
+ goto err;
}
/* Get the pointer's value. */
@@ -762,7 +752,7 @@ static int load_flat_file(struct linux_b
addr = calc_reloc(addr, libinfo, id, 0);
if (addr == RELOC_FAILED) {
ret = -ENOEXEC;
- goto err_close;
+ goto err;
}
/* Write back the relocated pointer. */
@@ -783,8 +773,6 @@ static int load_flat_file(struct linux_b
stack_len);
return 0;
-err_close:
- sys_close(exec_fileno);
err:
return ret;
}
_
On Tue, 16 May 2006, Bernd Schmidt wrote:
>
> It's not the get_file that's the problem, it's the get_unused_fd and
> fd_install. These files are now open while the process lives and consume file
> descriptors.
Side note: this would be a valid argument, except it's not always true.
I'm not sure why Luke wanted the fd in the first place, though, and
whether we want it.
Some loaders may actually want the fd value, see for example themisc
loader and MISC_FMT_OPEN_BINARY, and the ELF loader _does_ actually do it
for the (interpreter_type == INTERPRETER_AOUT) case.
So it's certainly not a new concept, and other loaders do the exact same
thing.
Whether the flat loader should do it (or under what circumstances it
should do it), I just can't make any judgement. More information needed.
> Before the change, we didn't allocate or install a file descriptor, hence
> there wasn't any reason to return EMFILE. The spec at
> http://www.opengroup.org/onlinepubs/009695399/functions/exec.html
> doesn't list EMFILE as a possible error.
Totally irrelevant.
A lot of system calls will return errors other than the ones listed. The
text says "shall fail if", which just means that those errors are
_required_ to happen under the circumstances listed (and you can't use
other errors _for_those_particular_circumstances_).
See
http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html#tag_02_03
"Implementations may support additional errors not included in this
list, may generate errors included in this list under circumstances
other than those described here, or may contain extensions or
limitations that prevent some errors from occurring. The ERRORS section
on each reference page specifies whether an error shall be returned, or
whether it may be returned. Implementations shall not generate a
different error number from the ones described here for error
conditions described in this volume of IEEE Std 1003.1-2001, but may
generate additional errors unless explicitly disallowed for a
particular function."
for more.
Linus
Andrew Morton wrote:
> It'd be nice not to lose the coding cleanups which that patch needed. Can
> you review-n-test this form of reversion?
I'll give it a spin. Thanks.
Bernd
Linus Torvalds wrote:
> Side note: this would be a valid argument, except it's not always true.
> I'm not sure why Luke wanted the fd in the first place, though, and
> whether we want it.
The only reason was a failed LTP testcase which fills up the FD table
and then called exec.
> Some loaders may actually want the fd value, see for example themisc
> loader and MISC_FMT_OPEN_BINARY, and the ELF loader _does_ actually do it
> for the (interpreter_type == INTERPRETER_AOUT) case.
The flat loader does not need a FD value.
>> Before the change, we didn't allocate or install a file descriptor, hence
>> there wasn't any reason to return EMFILE. The spec at
>> http://www.opengroup.org/onlinepubs/009695399/functions/exec.html
>> doesn't list EMFILE as a possible error.
>
> Totally irrelevant.
I think it is relevant: if the spec does not require it, and the flat
loader does not need the FD, then there is no reason to return EMFILE.
Both conditions are true in this case. If the spec did require it, then
that would be an argument that the LTP testcase is valid, and for
keeping the original patch.
Bernd