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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0C9F6CD3420 for ; Tue, 3 Sep 2024 11:24:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=w4IcuQeuiymqaS/a1RhzzDSLXJUOsYIq0tu0n3fb8ks=; b=Dur7pwUoo4zvKjE5bByqwweWCd dWIYofxCjedE2cyBX+gUIkpRGRYepfV7/eVJzamhlHROZeWuXFAd5R8PULSR4/hZ3iug1sPVquVOq qbqoQiZ/08ts3pyftxztjXTln9K7gPoYaM3Ap5sb8GWEuELLHIructzLVwAamoqQZFc7sran/Dohm sAKlpbhB2bar+4/nOhv+HbiNmIIpd27VIcpHbMzmHmepzjma2WBiH6hP8+axL44cu2Ht1Z/Jdx09p TTKLmTkmGcU7uD8DazDRbuk8SEPw2CuG/THjUGlU5d8e81hd+fAuiYu3p2j66yCl8+7HiOAE7ADgo ANrvx+EA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1slRdj-0000000HWIS-2TL5; Tue, 03 Sep 2024 11:24:03 +0000 Received: from mail-ej1-x62f.google.com ([2a00:1450:4864:20::62f]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1slRcl-0000000HW2x-2E7A for linux-mediatek@lists.infradead.org; Tue, 03 Sep 2024 11:23:07 +0000 Received: by mail-ej1-x62f.google.com with SMTP id a640c23a62f3a-a7aa086b077so442024166b.0 for ; Tue, 03 Sep 2024 04:23:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1725362580; x=1725967380; darn=lists.infradead.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=w4IcuQeuiymqaS/a1RhzzDSLXJUOsYIq0tu0n3fb8ks=; b=fSlmnWaJLlNElolUTi7ZBlSK8iGnsdyBZU93B2Ho+DYgvwNLYXV0T8O1TOzSZklV21 Z8Y+bCX465cQDPvbcFfvNRiBu6w1wf8HAEzQ/I84+xvjPDsRohHimEtF0J0B0344QsRu wE6VhxK3MH6dihyKqOGqh4BDsrNJlNV+1oJTJ1Q+UX+moN8ulGR7yhBZwLsdG5v0DadE 30N13bejYSvojv1hOp3UT/o9b/ejDUSPczsA1u1CAyrYOGkOONymZzaJljluIBW1WUCV BJ+Vx3p871/iR5q/Div8EdMOOfPcJ35eDKVfMrijuWoQ5osfP7RzgsaVvMFw2VjXsAgF DIOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725362580; x=1725967380; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=w4IcuQeuiymqaS/a1RhzzDSLXJUOsYIq0tu0n3fb8ks=; b=mRsyLSyTV8wrFVPs2XhHMw3z8fW3/Lm6Ikcy9353lSbwTDemEkdwYaKgL8s2Tv62ky VTHfhXNneaTWvSkGaDKpgGAHo61xYbE3lLzooPdKoE+eeKb7zNcA6Zi+ONcrF1O+ZE8N +nCEf1kl7zOMBM+yVfqei7UcHGj03+7pnD10drKn2eHNgns6C3Kngf96klaeKooUeb/T R8hosYMD98Q5M7oDRt5wsQ4UQ33RfNC5pNyaL7hc67mO2sL9Ls9koIwe+hjLqt9aLttr HV2aQDWzhWXiwodbawETWZm7Wp4v5mV/B4xN13KXCH+VQnQRJMwDTMH66VptosigO5P+ t/rg== X-Forwarded-Encrypted: i=1; AJvYcCVk8pBfxvkbdlcWqT15+iaZApSBDBgSZQ0bJUkS6rk8Mml8Jdx/l552swejxNgZhGjpJiiOlbh33chmMG1UiA==@lists.infradead.org X-Gm-Message-State: AOJu0Yzc+USW6+H3L5yg43411kvgUmdCs6UhhhB+VSdVup1/hNPDMcCS Uv6ywaNb6T1NEoIpMUJj4L3EQgimR92bCpTiB5+BbJEr2gXyCeAyTaCxoQBZeaCIUnSAfbywCVK 8S8c= X-Google-Smtp-Source: AGHT+IHuKeLghdg/ls1i2JFJtmfoByQgn6d7Wd00RW7FnQCnrsmyQ9cHM02wmJzDXMutnkx6CANOYw== X-Received: by 2002:a17:906:9c84:b0:a86:77ac:7e4e with SMTP id a640c23a62f3a-a89fae1b87emr516226866b.34.1725362579557; Tue, 03 Sep 2024 04:22:59 -0700 (PDT) Received: from [10.100.51.161] ([193.86.92.181]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a8988feae4dsm673991766b.31.2024.09.03.04.22.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 03 Sep 2024 04:22:59 -0700 (PDT) Message-ID: Date: Tue, 3 Sep 2024 13:22:58 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] module: abort module loading when sysfs setup suffer errors To: Chunhui Li Cc: Luis Chamberlain , Matthias Brugger , AngeloGioacchino Del Regno , linux-modules@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, wsd_upstream@mediatek.com, Xion Wang References: <20240830054400.26622-1-chunhui.li@mediatek.com> Content-Language: en-US From: Petr Pavlu In-Reply-To: <20240830054400.26622-1-chunhui.li@mediatek.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240903_042304_437960_29A6AFD2 X-CRM114-Status: GOOD ( 30.10 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On 8/30/24 07:43, Chunhui Li wrote: > When insmod a kernel module, if fails in add_notes_attrs or > add_sysfs_attrs such as memory allocation fail, mod_sysfs_setup > will still return success, but we can't access user interface > on android device. > > Patch for make mod_sysfs_setup can check the error of > add_notes_attrs and add_sysfs_attrs s/add_sysfs_attrs/add_sect_attrs/ I think it makes sense to propagate errors from these functions upward, although I wonder if the authors of this code didn't intentionally make the errors silent, possibly because the interface was mostly intended for debugging only? The original commits which added add_sect_attrs() and add_notes_attrs() don't mention anything explicitly in this regard: https://github.com/mpe/linux-fullhistory/commit/db939b519bea9b88ae1c95c3b479c0b07145f2a0 https://github.com/torvalds/linux/commit/6d76013381ed28979cd122eb4b249a88b5e384fa > > Signed-off-by: Xion Wang > Signed-off-by: Chunhui Li > --- > kernel/module/sysfs.c | 49 ++++++++++++++++++++++++++++++------------- > 1 file changed, 35 insertions(+), 14 deletions(-) > > diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c > index 26efe1305c12..a9ee650d995d 100644 > --- a/kernel/module/sysfs.c > +++ b/kernel/module/sysfs.c > @@ -69,12 +69,13 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs) > kfree(sect_attrs); > } > > -static void add_sect_attrs(struct module *mod, const struct load_info *info) > +static int add_sect_attrs(struct module *mod, const struct load_info *info) > { > unsigned int nloaded = 0, i, size[2]; > struct module_sect_attrs *sect_attrs; > struct module_sect_attr *sattr; > struct bin_attribute **gattr; > + int ret = 0; Nit: It isn't necessary to initialize this variable to 0 because the code explicitly does "return 0;" on success. While on the error path, the variable is always assigned. > > /* Count loaded sections and allocate structures */ > for (i = 0; i < info->hdr->e_shnum; i++) > @@ -85,7 +86,7 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info) > size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.bin_attrs[0]); > sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL); > if (!sect_attrs) > - return; > + return -ENOMEM; > > /* Setup section attributes. */ > sect_attrs->grp.name = "sections"; > @@ -103,8 +104,10 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info) > sattr->address = sec->sh_addr; > sattr->battr.attr.name = > kstrdup(info->secstrings + sec->sh_name, GFP_KERNEL); > - if (!sattr->battr.attr.name) > + if (!sattr->battr.attr.name) { > + ret = -ENOMEM; > goto out; > + } > sect_attrs->nsections++; > sattr->battr.read = module_sect_read; > sattr->battr.size = MODULE_SECT_READ_SIZE; > @@ -113,13 +116,16 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info) > } > *gattr = NULL; > > - if (sysfs_create_group(&mod->mkobj.kobj, §_attrs->grp)) > + if (sysfs_create_group(&mod->mkobj.kobj, §_attrs->grp)) { > + ret = -EIO; > goto out; > + } Why does the logic return -EIO instead of propagating the error code from sysfs_create_group()? > > mod->sect_attrs = sect_attrs; > - return; > + return 0; > out: > free_sect_attrs(sect_attrs); > + return ret; > } > > static void remove_sect_attrs(struct module *mod) > @@ -158,15 +164,16 @@ static void free_notes_attrs(struct module_notes_attrs *notes_attrs, > kfree(notes_attrs); > } > > -static void add_notes_attrs(struct module *mod, const struct load_info *info) > +static int add_notes_attrs(struct module *mod, const struct load_info *info) > { > unsigned int notes, loaded, i; > struct module_notes_attrs *notes_attrs; > struct bin_attribute *nattr; > + int ret = 0; Similarly here, the initialization is not necessary. > > /* failed to create section attributes, so can't create notes */ > if (!mod->sect_attrs) > - return; > + return -EINVAL; Since the patch modifies mod_sysfs_setup() to bail out when registering section attributes fails, this condition can no longer be true and the check can be removed. > > /* Count notes sections and allocate structures. */ > notes = 0; > @@ -176,12 +183,12 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info) > ++notes; > > if (notes == 0) > - return; > + return 0; > > notes_attrs = kzalloc(struct_size(notes_attrs, attrs, notes), > GFP_KERNEL); > if (!notes_attrs) > - return; > + return -ENOMEM; > > notes_attrs->notes = notes; > nattr = ¬es_attrs->attrs[0]; > @@ -201,19 +208,24 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info) > } > > notes_attrs->dir = kobject_create_and_add("notes", &mod->mkobj.kobj); > - if (!notes_attrs->dir) > + if (!notes_attrs->dir) { > + ret = -ENOMEM; > goto out; > + } > > for (i = 0; i < notes; ++i) > if (sysfs_create_bin_file(notes_attrs->dir, > - ¬es_attrs->attrs[i])) > + ¬es_attrs->attrs[i])) { > + ret = -EIO; > goto out; > + } Similarly here, the actual error from sysfs_create_bin_file() can be returned. > > mod->notes_attrs = notes_attrs; > - return; > + return 0; > > out: > free_notes_attrs(notes_attrs, i); > + return ret; > } > > static void remove_notes_attrs(struct module *mod) > @@ -385,11 +397,20 @@ int mod_sysfs_setup(struct module *mod, > if (err) > goto out_unreg_modinfo_attrs; > > - add_sect_attrs(mod, info); > - add_notes_attrs(mod, info); > + err = add_sect_attrs(mod, info); > + if (err) > + goto out_unreg_sect_attrs; > + > + err = add_notes_attrs(mod, info); > + if (err) > + goto out_unreg_notes_attrs; > > return 0; > > +out_unreg_notes_attrs: > + remove_notes_attrs(mod); > +out_unreg_sect_attrs: > + remove_sect_attrs(mod); Upon a failure from add_sect_attrs(), the caller doesn't need to unwind its operation. It is the responsibility of add_sect_attrs() to clean up after itself on error. Instead, the code in mod_sysfs_setup() needs to unwind all previous successful operations leading up to this point, which means here additionally to invoke del_usage_links(). I think you want something as follows: err = add_sect_attrs(mod, info); if (err) goto out_unreg_usage_links; err = add_notes_attrs(mod, info); if (err) goto out_unreg_sect_attrs; return 0; out_unreg_sect_attrs: remove_sect_attrs(mod); out_unreg_usage_links: del_usage_links(mod); [...] > out_unreg_modinfo_attrs: > module_remove_modinfo_attrs(mod, -1); > out_unreg_param: -- Cheers, Petr