Re: remove set_fs calls from the coredump code v6
Christoph Hellwig writes: > On Tue, May 05, 2020 at 03:28:50PM -0500, Eric W. Biederman wrote: >> We probably can. After introducing a kernel_compat_siginfo that is >> the size that userspace actually would need. >> >> It isn't something I want to mess with until this code gets merged, as I >> think the set_fs cleanups are more important. >> >> >> Christoph made some good points about how ugly the #ifdefs are in >> the generic copy_siginfo_to_user32 implementation. > > Take a look at the series you are replying to, the magic x86 ifdefs are > entirely gone from the common code :) Interesting. That is a different way of achieving that, and I don't hate it. I still want whatever you are doing to settle before I touch that code again. Removing the set_fs is important and I have other fish to fry at the moment. Eric
Re: remove set_fs calls from the coredump code v6
On Tue, May 05, 2020 at 03:28:50PM -0500, Eric W. Biederman wrote: > We probably can. After introducing a kernel_compat_siginfo that is > the size that userspace actually would need. > > It isn't something I want to mess with until this code gets merged, as I > think the set_fs cleanups are more important. > > > Christoph made some good points about how ugly the #ifdefs are in > the generic copy_siginfo_to_user32 implementation. Take a look at the series you are replying to, the magic x86 ifdefs are entirely gone from the common code :)
Re: remove set_fs calls from the coredump code v6
On Tue, May 05, 2020 at 10:42:58PM +0200, Christoph Hellwig wrote: > On Tue, May 05, 2020 at 09:34:46PM +0100, Al Viro wrote: > > Looks good. Want me to put it into vfs.git? #work.set_fs-exec, perhaps? > > Sounds good. Applied, pushed and added into #for-next
Re: remove set_fs calls from the coredump code v6
On Tue, May 05, 2020 at 09:34:46PM +0100, Al Viro wrote: > Looks good. Want me to put it into vfs.git? #work.set_fs-exec, perhaps? Sounds good.
Re: remove set_fs calls from the coredump code v6
On Tue, May 05, 2020 at 12:12:49PM +0200, Christoph Hellwig wrote: > Hi all, > > this series gets rid of playing with the address limit in the exec and > coredump code. Most of this was fairly trivial, the biggest changes are > those to the spufs coredump code. > > Changes since v5: > - fix uaccess under spinlock in spufs (Jeremy) > - remove use of access_ok in spufs > > Changes since v4: > - change some goto names as suggested by Linus > > Changes since v3: > - fix x86 compilation with x32 in the new version of the signal code > - split the exec patches into a new series > > Changes since v2: > - don't cleanup the compat siginfo calling conventions, use the patch >variant from Eric with slight coding style fixes instead. > > Changes since v1: > - properly spell NUL > - properly handle the compat siginfo case in ELF coredumps Looks good. Want me to put it into vfs.git? #work.set_fs-exec, perhaps?
Re: remove set_fs calls from the coredump code v6
Linus Torvalds writes:
> On Tue, May 5, 2020 at 3:13 AM Christoph Hellwig wrote:
>>
>> this series gets rid of playing with the address limit in the exec and
>> coredump code. Most of this was fairly trivial, the biggest changes are
>> those to the spufs coredump code.
>
> Ack, nice, and looks good.
>
> The only part I dislike is how we have that 'struct compat_siginfo' on
> the stack, which is a huge waste (most of it is the nasty padding to
> 128 bytes).
>
> But that's not new, I only reacted to it because the code moved a bit.
> We cleaned up the regular siginfo to not have the padding in the
> kernel (and by "we" I mean "Eric Biederman did it after some prodding
> as part of his siginfo cleanups" - see commit 4ce5f9c9e754 "signal:
> Use a smaller struct siginfo in the kernel"), and I wonder if we
> could do something similar with that compat thing.
>
> 128 bytes of wasted kernel stack isn't the end of the world, but it's
> sad when the *actual* data is only 32 bytes or so.
We probably can. After introducing a kernel_compat_siginfo that is
the size that userspace actually would need.
It isn't something I want to mess with until this code gets merged, as I
think the set_fs cleanups are more important.
Christoph made some good points about how ugly the #ifdefs are in
the generic copy_siginfo_to_user32 implementation.
I am thinking the right fix is to introduce.
- TS_X32 as a companion to TS_COMPAT in the x86_64.
- Modify in_x32_syscall() to test TS_X32
- Implement x32_copy_siginfo_to_user32 that forces TS_X32 to be
set. AKA:
x32_copy_siginfo_to_user32()
{
unsigned long state = current_thread_info()->state;
current_thread_info()->state |= TS_X32;
copy_siginfo_to_user32();
current_thread_info()->state = state;
}
That would make the #ifdefs go away, but I don't yet know what the x86
maintainers would say about that scheme. I think it is a good path as
it would isolate the runtime cost of that weird SIGCHLD siginfo format
to just x32. Then ia32 in compat mode would not need to pay.
Once I get that then it will be easier to introduce a yet another helper
of copy_siginfo_to_user32 that generates just the kernel_compat_siginfo
part, and the two visible derivatives can call memset and clear_user
to clear the unset parts.
I am assuming you don't don't mind having a full siginfo in
elf_note_info that ultimately gets copied into the core dump?
Eric
Re: remove set_fs calls from the coredump code v6
On Tue, May 5, 2020 at 3:13 AM Christoph Hellwig wrote: > > this series gets rid of playing with the address limit in the exec and > coredump code. Most of this was fairly trivial, the biggest changes are > those to the spufs coredump code. Ack, nice, and looks good. The only part I dislike is how we have that 'struct compat_siginfo' on the stack, which is a huge waste (most of it is the nasty padding to 128 bytes). But that's not new, I only reacted to it because the code moved a bit. We cleaned up the regular siginfo to not have the padding in the kernel (and by "we" I mean "Eric Biederman did it after some prodding as part of his siginfo cleanups" - see commit 4ce5f9c9e754 "signal: Use a smaller struct siginfo in the kernel"), and I wonder if we could do something similar with that compat thing. 128 bytes of wasted kernel stack isn't the end of the world, but it's sad when the *actual* data is only 32 bytes or so. Linus
remove set_fs calls from the coredump code v6
Hi all, this series gets rid of playing with the address limit in the exec and coredump code. Most of this was fairly trivial, the biggest changes are those to the spufs coredump code. Changes since v5: - fix uaccess under spinlock in spufs (Jeremy) - remove use of access_ok in spufs Changes since v4: - change some goto names as suggested by Linus Changes since v3: - fix x86 compilation with x32 in the new version of the signal code - split the exec patches into a new series Changes since v2: - don't cleanup the compat siginfo calling conventions, use the patch variant from Eric with slight coding style fixes instead. Changes since v1: - properly spell NUL - properly handle the compat siginfo case in ELF coredumps
