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

[Christophe Leroy]

Le 11/09/2021 à 17:58, Eric W. Biederman a écrit :
> Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:
>
> > On 9/8/21 6:17 PM, Eric W. Biederman wrote:
> > > Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:
> > >
> > > > Le 02/09/2021 à 20:43, Eric W. Biederman a écrit :
> > > > > Christophe Leroy <christophe.leroy@xxxxxxxxxx> 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