linux-kernel - Re: [PATCH] sched: make struct task

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: <92ead22e-0580-c720-1a29-7db79d76f7d7@arm.com>
Date:   Tue, 3 Sep 2019 22:51:30 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Alexey Dobriyan <adobriyan@...il.com>
Cc:     mingo@...hat.com, peterz@...radead.org,
        linux-kernel@...r.kernel.org, rcu@...r.kernel.org,
        linux-block@...r.kernel.org, dm-devel@...hat.com, axboe@...nel.dk,
        aarcange@...hat.com
Subject: Re: [PATCH] sched: make struct task_struct::state 32-bit

On 03/09/2019 19:19, Alexey Dobriyan wrote:
> On Tue, Sep 03, 2019 at 06:29:06PM +0100, Valentin Schneider wrote:
>> On 02/09/2019 22:05, Alexey Dobriyan wrote:
>>> 32-bit accesses are shorter than 64-bit accesses on x86_64.
>>> Nothing uses 64-bitness of ->state.
> 
>> It looks like you missed a few places. There's a long prev_state in
>> sched/core.c::finish_task_switch() for instance.
>>
>> I suppose that's where coccinelle oughta help but I'm really not fluent
>> in that. Is there a way to make it match p.state accesses with p task_struct?
>> And if so, can we make it change the type of the variable being read from
>> / written to?
> 
> Coccinelle is interesting: basic
> 
> 	- foo
> 	+ bar
> 
> doesn't find "foo" in function arguments.
> 
> I'm scared of coccinelle.> 

So am I, but I'm even more scared of having no other way of verifying this
than doing it "by hand". It's nothing critical here - just some variables
that will remain long instead of being int, but I'd like to have some way to
verify the change. A coccinelle script would be great, even if it misses
some places, I can at least have some trust in it.

If I have to verify the whole tree by hand, even with grep/ag, I don't trust
I'll do it right.

I gave Coccinelle a try, I think I got something for in-function variables:

---
@state_var@
struct task_struct *p;
identifier prev_state;
@@
prev_state = p->state

@@
identifier state_var.prev_state;
@@
- long prev_state;
+ int prev_state;
---

I tried something for function parameters, which seems to be feasible
according to [1], but couldn't get it to work (yet). Here's what I have
so far:

---
@func_param@
identifier func;
identifier p;
identifier state;
identifier mask;
@@

func(struct task_struct *p, const struct cpumask *mask, long state)
{
...
}

@@
identifier func_param.func;
identifier func_param.state;
expression E1;
expression E2;
@@

func(E1,
- long state,
+ int state,
E2)
---

(it should match against kernel/kthread.c::__kthread_bind_mask() but it
complains about me not knowing how to write coccinelle patches).

With a mix of these we might get somewhere...

[1]: http://coccinelle.lip6.fr/docs/main_grammar016.html

>> How did you come up with this changeset, did you pickaxe for some regexp?
> 
> No, manually, backtracking up to the call chain.
> Maybe I missed a few places.
>