LKML Archive on
help / color / mirror / Atom feed
From: Christophe Leroy <>
To: "Eric W. Biederman" <>
Cc: Benjamin Herrenschmidt <>,
	Paul Mackerras <>,
	Michael Ellerman <>,,
Subject: Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()
Date: Mon, 13 Sep 2021 14:56:25 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <87o88zqf3k.fsf@disp2133>

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

  reply	other threads:[~2021-09-13 12:56 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-23 15:35 [PATCH v2 1/5] powerpc/signal64: Access function descriptor with user access block Christophe Leroy
2021-08-23 15:35 ` [PATCH v2 2/5] powerpc/signal: Include the new stack frame inside the " Christophe Leroy
2021-08-23 15:35 ` [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user() Christophe Leroy
2021-09-02  6:54   ` Christoph Hellwig
2021-09-02 16:05     ` Linus Torvalds
2021-09-13 12:59     ` Christophe Leroy
2021-09-02 18:43   ` Eric W. Biederman
2021-09-03  8:56     ` Christophe Leroy
2021-09-08 18:17       ` Eric W. Biederman
2021-09-10 10:27         ` Christophe Leroy
2021-09-11 15:58           ` Eric W. Biederman
2021-09-13 12:56             ` Christophe Leroy [this message]
2021-08-23 15:35 ` [PATCH v2 4/5] powerpc/uaccess: Add unsafe_clear_user() Christophe Leroy
2021-08-23 15:35 ` [PATCH v2 5/5] powerpc/signal: Use unsafe_copy_siginfo_to_user() Christophe Leroy
2021-09-02 18:38   ` Eric W. Biederman
2021-09-03  8:53     ` Christophe Leroy
2021-09-02  6:49 ` [PATCH v2 1/5] powerpc/signal64: Access function descriptor with user access block Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).