linux-kernel - Re: [PATCH] task_work: cleanup notification modes

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: <aaed79d1-972f-e4bd-f3ac-d589cd729163@kernel.dk>
Date:   Fri, 16 Oct 2020 16:39:47 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] task_work: cleanup notification modes

On 10/16/20 3:44 PM, Thomas Gleixner wrote:
> On Fri, Oct 16 2020 at 09:16, Jens Axboe wrote:
>> A previous commit changed the notification mode from 0/1 to allowing
> 
> No. It changed it from boolean to an int.
> 
> There is a fundamental difference between 0/1 and false/true simply
> because it's a compiler implementation detail how to represent a boolean
> value.
> 
> Assume the following:
> 
> #define BAZ	0x08
> 
>        task_work_add(tsk, &work, foo & BAZ);
> 
> So with a function signature of
> 
>        task_work_add(*tsk, *work, bool signal);
> 
> the above 'foo & BAZ' becomes either true of false.
> 
> With the changed function signature of
> 
>        task_work_add(*tsk, *work, int signal);
> 
> the above becomes the result of 'foo & BAZ' which means that this
> construct will not longer do the right thing.
> 
> It's pure luck that none of the usage sites relied on the boolean
> property of that argument.

It wasn't pure luck, that was checked before that change was made. No
users did anything funky, it was all false/true or 0/1.

> Please spell it out correctly that converting a boolean argument to an
> integer argument is not equivalent.

Fixed up the commit message to be more descriptive.

>>  	switch (notify) {
>> +	case TWA_NONE:
>> +		break;
>>  	case TWA_RESUME:
>>  		set_notify_resume(task);
>>  		break;
> 
> The enum will not prevent that either and what you really want to do is
> to have some form of debug warning if 'notify' is out of range, which
> would have been the right thing to do in the first place.

I added a WARN_ON_ONCE() in the default case for that one.

>> - * @notify: send the notification if true
>> + * @notify: send chosen notification, if any
> 
> Is that really all you found to be wrong in that comment?

There really is nothing wrong, but it's not very descriptive (wasn't
before either). I've added a fuller description of the various TWA_*
notification types now.

-- 
Jens Axboe