On Sun, Apr 04, 2021 at 10:57:13AM -0300, Jason Gunthorpe wrote: > On Fri, Apr 02, 2021 at 06:29:55PM -0700, Kees Cook wrote: > > On Fri, Apr 02, 2021 at 08:30:18PM -0300, Jason Gunthorpe wrote: > > > On Fri, Apr 02, 2021 at 04:03:30PM -0700, Kees Cook wrote: > > > > > > > > relevant. It seems to me that the hw_counters 'struct attribute_group' > > > > > should probably be its own kobj within both of these structures so they > > > > > can have their own sysfs ops (unless there is some other way to do this > > > > > that I am missing). > > > > > > Err, yes, every subclass of the attribute should be accompanied by a > > > distinct kobject type to relay the show methods with typesafety, this > > > is how this design pattern is intended to be used. > > > > > > If I understand your report properly the hw_stats_attribute is being > > > assigned to a 'port_type' kobject and it only works by pure luck because > > > the show/store happens to overlap between port and hsa attributes? > > > > "happens to" :) Yeah, they're all like this, unfortunately: > > https://lore.kernel.org/lkml/202006112217.2E6CE093@keescook/ > > All? I think these are all bugs, no? > > struct kobj_attribute is only to be used with a kobj_sysfs_ops type > kobject > > To cross it over to a 'struct device' kobj is completely wrong, the > same basic wrongness being done here. > > > I'm not convinced that just backing everything off to kobject isn't > > simpler? > > It might be simpler, but isn't right - everything should continue to > work after a patch like this: > > --- a/drivers/infiniband/core/sysfs.c > +++ b/drivers/infiniband/core/sysfs.c > @@ -67,6 +67,7 @@ struct ib_port { > > struct port_attribute { > struct attribute attr; > + uu64 pad[2]; Ick, don't do that :( > ssize_t (*show)(struct ib_port *, struct port_attribute *, char *buf); > ssize_t (*store)(struct ib_port *, struct port_attribute *, > const char *buf, size_t count); > > If it doesn't it is still broken. > > Using container_of() with the wrong types is an unconditional > error. A kasn test to catch this would be very cool (think like RTTI > and dynamic_cast<>() in C++) > > > > And then two show/set functions that bounce through the correct types > > > to the data. > > > > I'd like to make these things compile-time safe (there is not type > > associated with use the __ATTR() macro, for example). That I haven't > > really figured out how to do right. > > 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. 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... thanks, greg k-h
