netdev - Re: [PATCH] Export SMBIOS provided firmware instance and label to sysfs

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: <20100710174938.GA22779@kroah.com>
Date:	Sat, 10 Jul 2010 10:49:38 -0700
From:	Greg KH <greg@...ah.com>
To:	Narendra K <Narendra_K@...l.com>
Cc:	netdev@...r.kernel.org, linux-hotplug@...r.kernel.org,
	linux-pci@...r.kernel.org, matt_domsch@...l.com,
	charles_rose@...l.com, jordan_hargrave@...l.com,
	vijay_nijhawan@...l.com
Subject: Re: [PATCH] Export SMBIOS provided firmware instance and label to
 sysfs

On Sat, Jul 10, 2010 at 12:14:45PM -0500, Narendra K wrote:
> +static char smbios_attr[4096];

Interesting size, too bad you don't reference it again:

> +			if (attribute == SMBIOS_ATTR_INSTANCE_SHOW)
> +				return scnprintf(smbios_attr, PAGE_SIZE,
> +						"%d\n", donboard->instance);
> +			else if (attribute == SMBIOS_ATTR_LABEL_SHOW)
> +				return scnprintf(smbios_attr, PAGE_SIZE,
> +						"%s\n", dmi->name);

PAGE_SIZE here?

Which is it (note, some arches PAGE_SIZE is not 4k...)

> +			return strlen(dmi->name);
> +		}
> +	}
> +	return 0;
> +}
> +
> +static ssize_t
> +smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	ssize_t result;
> +	result = smbios_instance_string_exist(&dev->kobj, &attr->attr,
> +					      SMBIOS_ATTR_LABEL_SHOW);
> +	strncpy(buf, smbios_attr, result);
> +	return result;
> +}
> +
> +static ssize_t
> +smbiosinstance_show(struct device *dev,
> +		    struct device_attribute *attr, char *buf)
> +{
> +	ssize_t result;
> +	result = smbios_instance_string_exist(&dev->kobj, &attr->attr,
> +					      SMBIOS_ATTR_INSTANCE_SHOW);
> +	strncpy(buf, smbios_attr, result);
> +	return result;
> +}
> +
> +static struct device_attribute smbios_attr_label = {
> +	.attr = {.name = "label", .mode = 0444, .owner = THIS_MODULE},
> +	.show = smbiosname_show,
> +};
> +
> +static struct device_attribute smbios_attr_instance = {
> +	.attr = {.name = "index", .mode = 0444, .owner = THIS_MODULE},
> +	.show = smbiosinstance_show,
> +};
> +
> +static struct attribute *smbios_attributes[] = {
> +	&smbios_attr_label.attr,
> +	&smbios_attr_instance.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group smbios_attr_group = {
> +	.attrs = smbios_attributes,
> +	.is_visible = smbios_instance_string_exist,
> +};
> +
> +static int
> +pci_create_smbiosname_file(struct pci_dev *pdev)
> +{
> +	if (smbios_attr_group.is_visible &&

You set .is_visable above, how could it not be set?

> +		smbios_attr_group.is_visible(&pdev->dev.kobj, NULL, 0)) {
> +		if (!sysfs_create_group(&pdev->dev.kobj, &smbios_attr_group))
> +			return 0;

No, sysfs_create_group will call .is_visable, right?  You shouldn't need
to call it yourself or check it, at all.

> +static int
> +pci_remove_smbiosname_file(struct pci_dev *pdev)
> +{
> +	if (smbios_attr_group.is_visible &&
> +		smbios_attr_group.is_visible(&pdev->dev.kobj, NULL, 0)) {
> +		sysfs_remove_group(&pdev->dev.kobj, &smbios_attr_group);
> +		return 0;

Same here, you shouldn't have to check .is_visable.

> +	}
> +	return -1;

Why -1?  Please use a real error value.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html