From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f50.google.com (mail-ej1-f50.google.com [209.85.218.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 792D61A0BF4 for ; Tue, 3 Sep 2024 11:23:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.50 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1725362583; cv=none; b=MSJKQKie+LKTnmqxJG0RwlzonjpbRTqkA2b+NjyJ4GPX72AeJmF9cSvyGgdgBywIczvd94mKZv35nrjxqHTnE8S4+2m+ojdBGXGgv5ZCQMKfTKPdogxHD1rQ5T30OT/TLBw99rZed0CuEbaqltWFvtrkAZk724t18dEPlhf+1iQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1725362583; c=relaxed/simple; bh=bJZp7v9VF/zV4FIdUkvtoSwQVPiQZ4CIEegB9jZ1ems=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Zw4LlDfLa4UZrh6D39gK8cfXf0TIrW+rWz8SPO7vn57a63YmA/J7pqMf9EPqbQaDdz/mfeoOzhEs8y61PjFyt7A8eNVNbd0hW1WcIgRy2ecCco85N2nxlpzP5nWQIGg7Z82fUNdctllPAwJ8DxJShN5Vu2oEMwSaYLEsKKU13nQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com; spf=pass smtp.mailfrom=suse.com; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b=PFT6Aiyp; arc=none smtp.client-ip=209.85.218.50 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=suse.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b="PFT6Aiyp" Received: by mail-ej1-f50.google.com with SMTP id a640c23a62f3a-a7a9cf7d3f3so572528266b.1 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=vger.kernel.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=PFT6Aiyphmh4O3azCZhqdkp3AU/wdEZCCJYVuMRA1izQyj7ADd3DmFtUUdKvvL7WzB WgoHOM1iQok10Sd0K0GWUEfiDW7E7m1H2P9sUIv8vZnGtsalgP86/SBobP1XK1Uc2M3G BRoS7QnatYgs9b+BnayTw/vgfK9i2OAxYpRkrIWT3v99o5o1bmYLSEVZLkD2aDuafOML sEkyc8tiHYluymJAQtU/USkD18czTSwOAfB1l6cLU7FQitHu3opFpegNYaWPNe8SOMFI r8u7NEx6QyRM0etVutufaFnsXGVpfJoeGxoYZXQl6p0DLYP0B2p3O6hP7As+rQtXY02m XC5Q== 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=V26Oj8tUmcEc3hBlRWg1Q8CsZfFpXscv8angjBt9nrdJqAo6tQ4+rlWoZo/FK4gxpX zzFn42pNicpWElDFb1d9awvneHt1zwZGfuu87m7fypcUZlw1ICu3HPe4AHNd2/ep1riw uCy29QrvSjKFZ+CLddmOt0yV/HAK9Ewi2u89kJutv9MwW0nMkYVR0YSie1p5+8TT3qpU 01U75oBh0UR0DS0raw8bRZZFPcevoBoGyT3O8yUDsHfOGIiBzj5egx9TV+1GU/EMuNAp PNTPIvQ4QGZG3MwUXXK/OFZmijJ7uDWKbQDQvP1VmG9sJPHJBjs/VEc2rcaRceABiH2j Htjw== X-Forwarded-Encrypted: i=1; AJvYcCXSJStliq+TmeCZRn6HhOGiz6ijI0wUmPUuRKvxDL390DsHFlQ5ipBbaUzo6FApqUU5UBMcplVK5yVyu/I=@vger.kernel.org X-Gm-Message-State: AOJu0YwTwgWqOccetAmBqEsVKaqXxWD08ksPale0K9dNGweBXfToq8sM dM8HnRjCW1SD3m9rblq6r5rCNC7EbaTwCPw5HgqgO/OYSoiA5lH66YJ+9KpaW8w= 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 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: 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 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