Messages in this thread | | | Subject | Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user() | From | Christophe Leroy <> | Date | Mon, 13 Sep 2021 14:56:25 +0200 |
| |
Le 11/09/2021 à 17:58, Eric W. Biederman a écrit : > Christophe Leroy <christophe.leroy@csgroup.eu> writes: > >> On 9/8/21 6:17 PM, Eric W. Biederman wrote: >>> Christophe Leroy <christophe.leroy@csgroup.eu> writes: >>> >>>> Le 02/09/2021 à 20:43, Eric W. Biederman a écrit : >>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes: >>>>> >>>>>> In the same spirit as commit fb05121fd6a2 ("signal: Add >>>>>> unsafe_get_compat_sigset()"), implement an 'unsafe' version of >>>>>> copy_siginfo_to_user() in order to use it within user access blocks. >>>>>> >>>>>> For that, also add an 'unsafe' version of clear_user(). >>>>> >>>>> Looking at your use cases you need the 32bit compat version of this >>>>> as well. >>>>> >>>>> The 32bit compat version is too complicated to become a macro, so I >>>>> don't think you can make this work correctly for the 32bit compat case. >>>> >>>> When looking into patch 5/5 that you nacked, I think you missed the fact that we >>>> keep using copy_siginfo_to_user32() as it for the 32 bit compat case. >>> >>> I did. My mistake. >>> >>> However that mistake was so easy I think it mirrors the comments others >>> have made that this looks like a maintenance hazard. >>> >>> Is improving the performance of 32bit kernels interesting? >> >> Yes it is, and that's what this series do. >> >>> Is improving the performance of 32bit compat support interesting? >> >> For me this is a corner case, so I left it aside for now. >> >>> >>> If performance one or either of those cases is interesting it looks like >>> we already have copy_siginfo_to_external32 the factor you would need >>> to build unsafe_copy_siginfo_to_user32. >> >> I'm not sure I understand your saying here. What do you expect me to >> do with copy_siginfo_to_external32() ? > > Implement unsafe_copy_siginfo_to_user32.
Ok, initialy I thought it would be a too big job but finaly that's not so big.
> >> copy_siginfo_to_user32() is for compat only. >> >> Native 32 bits powerpc use copy_siginfo_to_user() > > What you implemented doubles the number of test cases necessary to > compile test the 32bit ppc signal code, and makes the code noticeably > harder to follow.
Yes and no.
We already have a different copy_siginfo_to_user() for compat and for native, why would anything be doubled ?
I agree it makes the code harder to follow though
> > Having a unsafe_copy_to_siginfo_to_user32 at least would allow the > number of test cases to remain the same as the current code.
Not sure I follow you here, but regardless I have sent a v3 which tentatively implements copy_siginfo_to_user32() for the compat case.
> >>> So I am not going to say impossible but please make something >>> maintainable. I unified all of the compat 32bit siginfo logic because >>> it simply did not get enough love and attention when it was implemented >>> per architecture. >> >> Yes, and ? I didn't do any modification to the compat case, so what >> you did remains. > > You undid the unification between the 32bit code and the 32bit compat > code. > >>> In general I think that concern applies to this case as well. We really >>> need an implementation that shares as much burden as possible with other >>> architectures. >> >> I think yes, that's the reason why I made a generic >> unsafe_copy_siginfo_to_user() and didn't make a powerpc dedicated >> change. >> >> Once this is merged any other architecture can use >> unsafe_copy_siginfo_to_user(). >> >> Did I miss something ? > > Not dealing with the compat case and making the code signal stack frame > code noticeably more complicated. > > If this optimization profitably applies to other architectures we need > to figure out how to implement unsafe_copy_siginfo_to_user32 or risk > making them all much worse to maintain. >
Ok, let's see what you think about v3.
Thanks for you feedback Christophe
| |