linux-kernel - Re: [PATCH 2/7] cbd: introduce cbd

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 24 Apr 2024 16:43:47 +0800
From: Dongsheng Yang <dongsheng.yang@...ystack.cn>
To: Chaitanya Kulkarni <chaitanyak@...dia.com>,
 "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
 "axboe@...nel.dk" <axboe@...nel.dk>
Cc: "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
 Dongsheng Yang <dongsheng.yang.linux@...il.com>
Subject: Re: [PATCH 2/7] cbd: introduce cbd_transport



在 2024/4/24 星期三 下午 12:08, Chaitanya Kulkarni 写道:
>> +static ssize_t cbd_myhost_show(struct device *dev,
>> +			       struct device_attribute *attr,
>> +			       char *buf)
>> +{
>> +	struct cbd_transport *cbdt;
>> +	struct cbd_host *host;
>> +
>> +	cbdt = container_of(dev, struct cbd_transport, device);
>> +
>> +	host = cbdt->host;
>> +	if (!host)
>> +		return 0;
>> +
>> +	return sprintf(buf, "%d\n", host->host_id);
> 
> snprintf() ?

IMO, it will only print a decimal unsigned int, so it shouldn't overflow 
the buffer.
> 
>> +}
>> +
>> +static DEVICE_ATTR(my_host_id, 0400, cbd_myhost_show, NULL);
>> +
>> +enum {
> 
> [...]
> 
>> +
>> +static ssize_t cbd_adm_store(struct device *dev,
>> +				 struct device_attribute *attr,
>> +				 const char *ubuf,
>> +				 size_t size)
>> +{
>> +	int ret;
>> +	char *buf;
>> +	struct cbd_adm_options opts = { 0 };
>> +	struct cbd_transport *cbdt;
>> +
> 
> reverse tree order that matches rest of your code ?

Agreed,
> 
>> +	if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	cbdt = container_of(dev, struct cbd_transport, device);
>> +
>> +	buf = kmemdup(ubuf, size + 1, GFP_KERNEL);
>> +	if (IS_ERR(buf)) {
>> +		pr_err("failed to dup buf for adm option: %d", (int)PTR_ERR(buf));
>> +		return PTR_ERR(buf);
>> +	}
>> +	buf[size] = '\0';
>> +	ret = parse_adm_options(cbdt, buf, &opts);
>> +	if (ret < 0) {
>> +		kfree(buf);
>> +		return ret;
>> +	}
>> +	kfree(buf);
>> +
> 
> standard format is using goto out and having only on kfree()
> at the end of the function ...

Okey, having a unified error handling path is a good idea, and it's 
suitable here as well, thanx.
> 
>> +	switch (opts.op) {
>> +	case CBDT_ADM_OP_B_START:
>> +		break;
>> +	case CBDT_ADM_OP_B_STOP:
>> +		break;
>> +	case CBDT_ADM_OP_B_CLEAR:
>> +		break;
>> +	case CBDT_ADM_OP_DEV_START:
>> +		break;
>> +	case CBDT_ADM_OP_DEV_STOP:
>> +		break;
>> +	default:
>> +		pr_err("invalid op: %d\n", opts.op);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return size;
>> +}
>> +
> 
> [...]
> 
>> +static struct cbd_transport *cbdt_alloc(void)
>> +{
>> +	struct cbd_transport *cbdt;
>> +	int ret;
>> +
>> +	cbdt = kzalloc(sizeof(struct cbd_transport), GFP_KERNEL);
>> +	if (!cbdt) {
>> +		return NULL;
>> +	}
> 
> no braces needed for single statements in if ... applies rest of
> the code ...

thanx, Iwill remove unnecessary braces next version.
> 
> -ck
> 
>