linux-kernel - Re: [PATCH v4 2/2] livepatch: Add using attribute to klp

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: <B250EB77-AFB0-4D32-BA4E-3B96976F8A82@gmail.com>
Date: Sun, 8 Sep 2024 10:51:14 +0800
From: zhang warden <zhangwarden@...il.com>
To: Petr Mladek <pmladek@...e.com>
Cc: Miroslav Benes <mbenes@...e.cz>,
 Josh Poimboeuf <jpoimboe@...nel.org>,
 Jiri Kosina <jikos@...nel.org>,
 Joe Lawrence <joe.lawrence@...hat.com>,
 live-patching@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for
 using function show


Hi, Petr
> 
> The 1st patch adds the pointer to struct klp_ops into struct
> klp_func. We might check the state a similar way as klp_ftrace_handler().
> 
> I had something like this in mind when I suggested to move the pointer:
> 
> static ssize_t using_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
> struct klp_func *func, *using_func;
> struct klp_ops *ops;
> int using;
> 
> func = container_of(kobj, struct klp_func, kobj);
> 
> rcu_read_lock();
> 
> if (func->transition) {
> using = -1;
> goto out;
> }
> 
> # FIXME: This requires releasing struct klp_ops via call_rcu()
> ops = func->ops;
> if (!ops) {
> using = 0;
> goto out;
> }
> 
> using_func = list_first_or_null_rcu(&ops->func_stack,
> struct klp_func, stack_node);
> if (func == using_func)
> using = 1;
> else
> using = 0;
> 
> out:
> rcu_read_unlock();
> 
> return sysfs_emit(buf, "%d\n", func->using);
> }

Bravo, I also have something like this in mind when Miroslav suggested to implement the logic in using_show. 

> 
> It is racy and tricky. We probably should add some memory barriers.
> And maybe even the ordering of reads should be different.
> 
> We could not take klp_mutex because it might cause a deadlock when
> the sysfs file gets removed. kobject_put(&func->kobj) is called
> by __klp_free_funcs() under klp_mutex.
> 
> It would be easier if we could take klp_mutex. But it would require
> decrementing the kobject refcout without of klp_mutex. It might
> be complicated.
> 
> I am afraid that this approach is not worth the effort and
> is is not the way to go.
> 

But I don't think I've thought as deeply as you do and may not have considered the possible risks. Therefore, I do need your help to make my patch perfect.  ^_^

Thank you.
Wardenjohn.