From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E4BB7C433F5 for ; Fri, 5 Nov 2021 08:01:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CC5B3604DA for ; Fri, 5 Nov 2021 08:01:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232487AbhKEIEC (ORCPT ); Fri, 5 Nov 2021 04:04:02 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]:48846 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232356AbhKEIEB (ORCPT ); Fri, 5 Nov 2021 04:04:01 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1636099281; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=0MqXgdVfhBySSGVsmZdlv09NxUwfcfOPShoRaoY0Oy8=; b=cV0v+18rwWRV02WjMSBEpl0IwPtVR3ukC5K652LX40NkCTucb+maRgCNOBaKh2ILiCyJVG TeXXXtgIG7lKT25Y8SUsbYCdP/2rwHrLU9F9ovz4QAQu0MDdHmLlR4ZQlfMIYRLwBJkuYJ fW0RfcNKi9dhkrhxVYE2OeVNCssn88k= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-37-LrDIzKHvMBq5Fy_EvqV5AQ-1; Fri, 05 Nov 2021 04:01:20 -0400 X-MC-Unique: LrDIzKHvMBq5Fy_EvqV5AQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id F24CF9126F; Fri, 5 Nov 2021 08:01:18 +0000 (UTC) Received: from T590 (ovpn-8-17.pek2.redhat.com [10.72.8.17]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2D4035D9DE; Fri, 5 Nov 2021 08:00:01 +0000 (UTC) Date: Fri, 5 Nov 2021 15:59:56 +0800 From: Ming Lei To: Petr Mladek Cc: Josh Poimboeuf , Jiri Kosina , Miroslav Benes , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Luis Chamberlain , Joe Lawrence , ming.lei@redhat.com Subject: Re: [PATCH V4 3/3] livepatch: free klp_patch object synchronously Message-ID: References: <20211102145932.3623108-1-ming.lei@redhat.com> <20211102145932.3623108-4-ming.lei@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 03, 2021 at 02:55:19PM +0100, Petr Mladek wrote: > 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(). The protection needs to cover removing both 'enabled' attribute and the patch kobject, so sysfs_remove_file_self() isn't good here. > > 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. It is used by generic pci device and scsi device, both are the most popular devices in the world, either one of the two subsystem should have huge amount of users, so it means the interface itself has been proved/verified for long time by many enough real users. > > 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 ;-) The fundamental advantage is that the API has been used/verified by enough real users. Also killing attribute/kobject itself isn't unique for livepatch, that is actually one common pattern, so it needn't such hacky implementation. Thanks, Ming