All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Vincenzo Frascino <Vincenzo.Frascino@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH] arm64/traps: Avoid unnecessary kernel/user pointer conversion
Date: Tue, 14 Sep 2021 17:00:56 +0100	[thread overview]
Message-ID: <20210914160056.GA35239@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20210914152742.27047-1-amit.kachhap@arm.com>

On Tue, Sep 14, 2021 at 08:57:42PM +0530, Amit Daniel Kachhap wrote:
> Annotating a pointer from kernel to __user and then back again might
> confuse sparse. In call_undef_hook() it can be avoided by not using the
> intermediate user pointer variable.

When you say "might confuse sparse", does it complain today? If so, can
you include an example of what goes wrong?

> Note: This patch adds no functional changes to code.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> ---
>  arch/arm64/kernel/traps.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index b03e383d944a..357d10a8bbf5 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -404,7 +404,8 @@ static int call_undef_hook(struct pt_regs *regs)
>  
>  	if (!user_mode(regs)) {
>  		__le32 instr_le;
> -		if (get_kernel_nofault(instr_le, (__force __le32 *)pc))
> +		if (get_kernel_nofault(instr_le,
> +				       (__le32 *)instruction_pointer(regs)))

Can we make `pc` an unsigned long, instead?

It'd be nice to handle all three cases consistently, even if that means
adding __force to the two user cases.

Thanks,
Mark.

>  			goto exit;
>  		instr = le32_to_cpu(instr_le);
>  	} else if (compat_thumb_mode(regs)) {
> -- 
> 2.17.1
> 

WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com>
To: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Vincenzo Frascino <Vincenzo.Frascino@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH] arm64/traps: Avoid unnecessary kernel/user pointer conversion
Date: Tue, 14 Sep 2021 17:00:56 +0100	[thread overview]
Message-ID: <20210914160056.GA35239@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20210914152742.27047-1-amit.kachhap@arm.com>

On Tue, Sep 14, 2021 at 08:57:42PM +0530, Amit Daniel Kachhap wrote:
> Annotating a pointer from kernel to __user and then back again might
> confuse sparse. In call_undef_hook() it can be avoided by not using the
> intermediate user pointer variable.

When you say "might confuse sparse", does it complain today? If so, can
you include an example of what goes wrong?

> Note: This patch adds no functional changes to code.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> ---
>  arch/arm64/kernel/traps.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index b03e383d944a..357d10a8bbf5 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -404,7 +404,8 @@ static int call_undef_hook(struct pt_regs *regs)
>  
>  	if (!user_mode(regs)) {
>  		__le32 instr_le;
> -		if (get_kernel_nofault(instr_le, (__force __le32 *)pc))
> +		if (get_kernel_nofault(instr_le,
> +				       (__le32 *)instruction_pointer(regs)))

Can we make `pc` an unsigned long, instead?

It'd be nice to handle all three cases consistently, even if that means
adding __force to the two user cases.

Thanks,
Mark.

>  			goto exit;
>  		instr = le32_to_cpu(instr_le);
>  	} else if (compat_thumb_mode(regs)) {
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-09-14 16:01 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-14 15:27 [PATCH] arm64/traps: Avoid unnecessary kernel/user pointer conversion Amit Daniel Kachhap
2021-09-14 15:27 ` Amit Daniel Kachhap
2021-09-14 16:00 ` Mark Rutland [this message]
2021-09-14 16:00   ` Mark Rutland
2021-09-15 13:56   ` Amit Kachhap
2021-09-15 13:56     ` Amit Kachhap

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=20210914160056.GA35239@C02TD0UTHF1T.local \
    --to=mark.rutland@arm.com \
    --cc=Vincenzo.Frascino@arm.com \
    --cc=amit.kachhap@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.