netdev - Re: [patch net-next 09/10] netdevsim: add ACL trap reporting cookie as a metadata

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]

Message-ID: <20200224204257.07c7456f@cakuba.hsd1.ca.comcast.net>
Date:   Mon, 24 Feb 2020 20:42:57 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, nhorman@...driver.com,
        jhs@...atatu.com, xiyou.wangcong@...il.com, idosch@...lanox.com,
        mlxsw@...lanox.com
Subject: Re: [patch net-next 09/10] netdevsim: add ACL trap reporting cookie
 as a metadata

On Mon, 24 Feb 2020 22:07:57 +0100, Jiri Pirko wrote:
> +static ssize_t nsim_dev_trap_fa_cookie_write(struct file *file,
> +					     const char __user *data,
> +					     size_t count, loff_t *ppos)
> +{
> +	struct nsim_dev *nsim_dev = file->private_data;
> +	struct flow_action_cookie *fa_cookie;
> +	size_t cookie_len = count / 2;
> +	ssize_t ret;
> +	char *buf;
> +
> +	if (*ppos != 0)
> +		return 0;

return 0? Should this not be an error?

> +	cookie_len = (count - 1) / 2;

why was cookie_len initialized when it was declared? 

> +	if ((count - 1) % 2)
> +		return -EINVAL;
> +	buf = kmalloc(count, GFP_KERNEL);

Strangely the malloc below has a NOWARN, but this one doesn't?

> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = simple_write_to_buffer(buf, count, ppos, data, count);
> +	if (ret < 0)
> +		goto err_write_to_buffer;
> +
> +	fa_cookie = kmalloc(sizeof(*fa_cookie) + cookie_len,
> +			    GFP_KERNEL | __GFP_NOWARN);
> +	if (!fa_cookie) {
> +		ret = -ENOMEM;
> +		goto err_alloc_cookie;
> +	}
> +
> +	fa_cookie->cookie_len = cookie_len;
> +	ret = hex2bin((u8 *) fa_cookie->cookie, buf, cookie_len);

this u8 cast won't be necessary if type of cookie changes :)

Also I feel like we could just hold onto the ASCII hex buf, 
and simplify the reading side. If the hex part is needed in 
the first place, hexdump and xxd exist..

> +	if (ret)
> +		goto err_hex2bin;
> +	kfree(buf);
> +
> +	spin_lock(&nsim_dev->fa_cookie_lock);
> +	kfree(nsim_dev->fa_cookie);
> +	nsim_dev->fa_cookie = fa_cookie;
> +	spin_unlock(&nsim_dev->fa_cookie_lock);
> +
> +	return count;
> +
> +err_hex2bin:
> +	kfree(fa_cookie);
> +err_alloc_cookie:
> +err_write_to_buffer:

Error labels should be named after what they undo, not after
destination. That makes both the source and target of the jump 
easy to review.

> +	kfree(buf);
> +	return ret;
> +}