linux-kernel - Re: [PATCH] aoe: Use IS_ERR_OR

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] [day] [month] [year] [list]

Message-ID: <20240823073055.GD1049718@ZenIV>
Date: Fri, 23 Aug 2024 08:30:55 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: zhangjiao2 <zhangjiao2@...s.chinamobile.com>
Cc: axboe@...nel.dk, justin@...aid.com, linux-block@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] aoe: Use IS_ERR_OR_NULL() to clean code

On Fri, Aug 23, 2024 at 01:26:40PM +0800, zhangjiao2 wrote:
> From: Zhang Jiao <zhangjiao2@...s.chinamobile.com>
> 
> Use IS_ERR_OR_NULL() to make the code cleaner.

ITYM "obfuscate the bogus code".

Take a look at kthread_run() definition:

#define kthread_run(threadfn, data, namefmt, ...)                          \
({                                                                         \
        struct task_struct *__k                                            \
                = kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
        if (!IS_ERR(__k))                                                  \
                wake_up_process(__k);                                      \
        __k;                                                               \
})

OK, what would need to happen for that to return NULL?  kthread_create()
returning NULL *AND* wake_up_process(NULL) surviving, right?

int wake_up_process(struct task_struct *p)
{
        return try_to_wake_up(p, TASK_NORMAL, 0);
}

OK, so we'd need try_to_wake_up(NULL, ...) to survive execution:

int try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
{
        guard(preempt)();
        int cpu, success = 0;

        if (p == current) {
		whatever, current is never NULL or a lot of places would be
		utterly screwed
	}
	/* some comment */
        scoped_guard (raw_spinlock_irqsave, &p->pi_lock) {

... and that would start with trying to grab &NULL->pi_lock, which is not
going to survive.

>  	task = kthread_run(kthread, k, "%s", k->name);
> -	if (task == NULL || IS_ERR(task))
> +	if (IS_ERR_OR_NULL(task))
>  		return -ENOMEM;

In other words, task == NULL had been pointless all along.  Your change only
makes it harder to spot.

IS_ERR_OR_NULL is almost never the right thing to do; there are cases where
a function may legitimately return a pointer to object, NULL *or* ERR_PTR(something),
but most of the time it's either impossible (and the caller couldn't have been
arsed to check what the calling conventions are) or a sign of a function in
bad need of saner calling conventions.

In this case it's the former - kthread_run() never returns a NULL and
actually if you look into kthread_create() you'll see that it returns
a pointer to new task_struct instance on success and ERR_PTR(-E...) on
failure.  NULL is never returned by that thing.

This kind of "defensive" programming only confuses the readers; please,
don't paper over that garbage.