Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 15 May 2023 21:17:39 -0700
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     Joel Granados <j.granados@samsung.com>
Cc:     Sudip Mukherjee <sudipm.mukherjee@gmail.com>,
        Iurii Zaikin <yzaikin@google.com>,
        Kees Cook <keescook@chromium.org>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 2/6] parport: Remove register_sysctl_table from
 parport_proc_register
Message-ID: <ZGMD4xMRKS8dZJpU@bombadil.infradead.org>
References: <20230515071446.2277292-1-j.granados@samsung.com>
 <CGME20230515071450eucas1p1625a8639e2b0edf47e41126801d4cbb8@eucas1p1.samsung.com>
 <20230515071446.2277292-3-j.granados@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230515071446.2277292-3-j.granados@samsung.com>
Sender: Luis Chamberlain <mcgrof@infradead.org>
Precedence: bulk

Awesome!

On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> +
> +	port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> +	/*
> +	 * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> +	 * We calculate for the second as that will give us enough for the first.
> +	 */
> +	tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> +	tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);

Any reason why not kzalloc()?

> +	if (!tmp_dir_path) {
> +		err = -ENOMEM;
> +		goto exit_free_t;
> +	}
>  
> -	t->port_dir[0].procname = port->name;
> +	if (tmp_path_len
> +	    <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {

Since we are clearing up obfuscation code, it would be nicer to
make this easier to read and split the snprintf() into one line, capture
the error there. And then in a new line do the check. Even if we have to
add a new int value here.

Other than this I'd just ask to extend the commit log to use
the before and after of vmlinux (when this module is compiled in with all
the bells and whistles) with ./scripts/bloat-o-meter.

Ie build before the patch and cp vmlinux to vmlinux.old and then compare
with:

./scripts/bloat-o-meter vmlinux.old vmlinux

Can you also describe any testing if any.

With the above changes, feel free to add to all these patches:

Reviewed-by: Luis Chamberlain

> +	if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)

BTW, we should be able to remove now replace register_sysctl_base() with a simple
register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
on kernel/sysctl.c and just remove:

  * DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
  * and then after all this register_sysctl_table() completely

Let me know if you'd like a stab at it, or if you prefer me to do that.

  Luis