linux-kernel - Re: [PATCH 2/2] MIPS: kprobes: Massage previous delay slot detection

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <b4833f82-9e7e-4667-994b-c444ef935a9f@app.fastmail.com>
Date: Tue, 10 Sep 2024 10:43:23 +0100
From: "Jiaxun Yang" <jiaxun.yang@...goat.com>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
Cc: "Thomas Bogendoerfer" <tsbogend@...ha.franken.de>,
 "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] MIPS: kprobes: Massage previous delay slot detection

在2024年9月9日九月 下午11:02，Maciej W. Rozycki写道：
> On Sun, 8 Sep 2024, Jiaxun Yang wrote:
>
>> Expand the if condition into cascaded ifs to make code
>> readable.
>
>  Apart from broken formatting what's making original code unreadable?

For me it's confusing because wired layout, cascaded ifs are clearly
easier to format and has clear intention.

>
>> Also use sizeof(union mips_instruction) instead of
>> sizeof(mips_instruction) to match the code context.
>
>  That has to be a separate change.

Given that it's a tiny style change as well, it makes sense to combine
into same patch.

>
>> diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
>> index dc39f5b3fb83..96139adefad2 100644
>> --- a/arch/mips/kernel/kprobes.c
>> +++ b/arch/mips/kernel/kprobes.c
>> @@ -89,12 +89,12 @@ int arch_prepare_kprobe(struct kprobe *p)
>>  		goto out;
>>  	}
>>  
>> -	if (copy_from_kernel_nofault(&prev_insn, p->addr - 1,
>> -			sizeof(mips_instruction)) == 0 &&
>> -	    insn_has_delayslot(prev_insn)) {
>> -		pr_notice("Kprobes for branch delayslot are not supported\n");
>> -		ret = -EINVAL;
>> -		goto out;
>> +	if (!copy_from_kernel_nofault(&prev_insn, p->addr - 1, sizeof(union mips_instruction))) {
>
>  Overlong line.

Nowadays, check-patch.pl is happy with 100 column line.

I used 100 column line in many subsystems and never receive any complaint.

>
>> +		if (insn_has_delayslot(prev_insn)) {
>> +			pr_notice("Kprobes for branch delayslot are not supported\n");
>
>  This now overruns 80 columns making code *less* readable.

I don't really agree, we are not in VGA display era any more, see Linus's
arguments on removal of 80 columns [1] and why long line are more readable [2].

[1]: https://lore.kernel.org/lkml/CAHk-=wj3iGQqjpvc+gf6+C29Jo4COj6OQQFzdY0h5qvYKTdCow@mail.gmail.com/
[2]: https://lore.kernel.org/lkml/CAHk-=wjR0H3+2ba0UUWwoYzYBH0GX9yTf5dj2MZyo0xvyzvJnA@mail.gmail.com/

Thanks
>
>   Maciej

-- 
- Jiaxun