Re: Buggy __free(kfree) usage pattern already in tree
From: Peter Zijlstra
Date: Fri Sep 15 2023 - 17:09:51 EST
On Fri, Sep 15, 2023 at 01:40:25PM -0700, Linus Torvalds wrote:
> Not because I think it's necessarily any kind of final rule, but
> because I think our whole cleanup thing is new enough that I think
> we're better off being a bit inflexible, and having a syntax where a
> simple "grep" ends up showing pretty much exactly what is going on wrt
> the pairing.
So in the perf-event conversion patches I do have this:
struct task_struct *task __free(put_task) = NULL;
...
if (pid != -1) {
task = find_lively_task_by_vpid(pid);
if (!task)
return -ESRCH;
}
...
pattern. The having of task is fully optional in the code-flow.
I suppose I can try and rewrite that a little something like:
...
struct task_struct *task __free(put_task) =
find_lively_task_by_vpid(pid); /* ensure pid==-1 returns NULL */
if (!task && pid > 0)
return -ESRCH;
...
But a little later in that same function I then have:
do {
struct rw_semaphore *exec_update_lock __free(up_read) = NULL;
if (task) {
err = down_read_interruptible(&task->signal->exec_update_lock);
if (err)
return err;
exec_update_lock = &task->signal->exec_update_lock;
if (!perf_check_permissions(&attr, task))
return -EACCESS;
}
... stuff serialized against exec *if* this is a task event ...
} while (0);
And that might be a little harder to 'fix'.
I suppose I'm saying that when thing truly are conditional, this is a
useful pattern, but avoid where reasonably possible.