linux-kernel - Re: [PATCH V4 3/3] livepatch: free 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]

Date:   Wed, 3 Nov 2021 14:55:19 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Ming Lei <ming.lei@...hat.com>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>, live-patching@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Joe Lawrence <joe.lawrence@...hat.com>
Subject: Re: [PATCH V4 3/3] livepatch: free klp_patch object synchronously

On Tue 2021-11-02 22:59:32, Ming Lei wrote:
> klp_mutex isn't acquired before calling kobject_put(klp_patch), so it is
> fine to free klp_patch object synchronously.
> 
> One issue is that enabled store() method, in which the klp_patch kobject
> itself is deleted & released. However, sysfs has provided APIs for dealing
> with this corner case, so use sysfs_break_active_protection() and
> sysfs_unbreak_active_protection() for releasing klp_patch kobject from
> enabled_store(), meantime the enabled attribute has to be removed
> before deleting the klp_patch kobject.
> 
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -369,10 +370,18 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  out:
>  	mutex_unlock(&klp_mutex);
>  
> -	klp_free_patches_async(&to_free);
> -
>  	if (ret)
>  		return ret;
> +
> +	if (!list_empty(&to_free)) {
> +		kn = sysfs_break_active_protection(kobj, &attr->attr);
> +		WARN_ON_ONCE(!kn);
> +		sysfs_remove_file(kobj, &attr->attr);
> +		klp_free_patches(&to_free);
> +		if (kn)
> +			sysfs_unbreak_active_protection(kn);
> +	}

I agree that using workqueues for free_work looks like a hack.
But this looks even more tricky and fragile to me. It feels like
playing with sysfs/kernfs internals.

It might look less tricky when using sysfs_remove_file_self().

Anyway, there are only few users of these APIs:

   + sysfs_break_active_protection() is used only scsi
   + kernfs_break_active_protection() is used by cgroups, cpusets, and rdtgroup.
   + sysfs_remove_file_self() is used by some RDMA-related stuff.

It means that there are some users but it is not widely used API.

I would personally prefer to keep it as is. I do not see any
fundamental advantage of the new code. But I might be biased
because the current code was written by me ;-)

Best Regards,
Petr