Messages in this thread | | | Date | Wed, 5 May 2021 14:29:16 -0300 | From | Jason Gunthorpe <> | Subject | Re: CFI violation in drivers/infiniband/core/sysfs.c |
| |
On Wed, May 05, 2021 at 06:26:06PM +0200, Greg KH wrote: > > They are in many places, for instance. > > > > int device_create_file(struct device *dev, > > const struct device_attribute *attr) > > > > We loose the type safety when working with attribute arrays, and > > people can just bypass the "proper" APIs to raw sysfs ones whenever > > they like. > > > > It is fundamentally completely wrong to attach a 'struct > > kobject_attribute' to a 'struct device' kobject. > > But it works because we are using C and we don't have RTTI :) > > Yes, it's horrid, but we do it because we "know" the real type that is > being called here. That was an explicit design decision at the time.
I think it is beyond horrid. Just so everyone is clear on what is happening here..
RDMA has this:
struct hw_stats_attribute { struct attribute attr; ssize_t (*show)(struct kobject *kobj, struct attribute *attr, char *buf);
And it has two kobject types, a struct device kobject and a ib_port kobject.
When the user invokes show on the struct device sysfs we have this call path:
dev_sysfs_ops dev_attr_show() struct device_attribute *dev_attr = to_dev_attr(attr); ret = dev_attr->show(dev, dev_attr, buf); show_hw_stats() struct hw_stats_attribute *hsa = container_of(attr, struct hw_stats_attribute, attr)
And from the ib_port kobject we have this one:
port_sysfs_ops port_attr_show() struct port_attribute *port_attr = container_of(attr, struct port_attribute, attr); return port_attr->show(p, port_attr, buf); show_hw_stats() struct hw_stats_attribute *hsa = container_of(attr, struct hw_stats_attribute, attr)
Then show_hw_stats() goes on to detect which call chain it uses so it can apply the proper container of to the kobj:
if (!hsa->port_num) dev = container_of((struct device *)kobj, struct ib_device, dev); else port = container_of(kobj, struct ib_port, kobj);
There are several nasty defeats of the C typing system here:
- A hw_stats_attribute is casted to device_attribute hidden inside container_of()
- The 'show' function pointer is being casted from from a (*show)(kobject,attr,buf) to (*show)(device,device_attr,buf) This cast is hidden by the above wrong use of container_of()
- The dev_attr 'struct device_attribute *' is casted directly to a 'struct attribute *' and this cast is hidden because of the wrongly type function pointer
- The dev 'struct device *' is casted directly to a 'struct kobject *' and like above this is hidden inside the wrongly typed function pointer.
- All of the above is true again when talking about port_attribute and struct ib_port.
This all implicitly relies on the following unchecked and undocumated relationships: - struct device's kobject is the first member in the struct - struct ib_port's kobject is the first member in the struct - The attr, show and store members are at the same offset in struct device_attribute and struct hw_stats_attribute
None of this is even slightly clear from the code. If Nathan hadn't pointed it out I don't think anyone would have known..
> If that was a good decision or not, I don't know, but it's served us > well for the past 20 years or so...
I agree with Kees, "my mind rebelled". I don't think it aligned with the modern kernel style. If tooling starts to shine light on these bast casts I feel it would only improve code quality.
For instance the patch Kees pointed at e6d701dca989 ("ACPI: sysfs: Fix pm_profile_attr type")
This is a legitimate typing bug. ACPI should not have been using struct device_attribute with a kobject creted by
acpi_kobj = kobject_create_and_add("acpi", firmware_kobj);
Certainly this RDMA code has no buisness being written like this either, it nets out to saving about 50 lines of straightforward duplicated code for a lot of worse junk.
Regards, Jason
| |