Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

On 9/8/21 6:17 PM, Eric W. Biederman wrote:

Christophe Leroy [off-list ref] writes:

quoted

Le 02/09/2021 à 20:43, Eric W. Biederman a écrit :

quoted

Christophe Leroy [off-list ref] writes:

quoted

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() ?

copy_siginfo_to_user32() is for compat only.

Native 32 bits powerpc use copy_siginfo_to_user()

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.

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 ?

Christophe