| Message ID | 20210918150500.21530-1-len.baker@gmx.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic | expand |
On Sat, Sep 18, 2021 at 05:05:00PM +0200, Len Baker wrote: > As noted in the "Deprecated Interfaces, Language Features, Attributes, > and Conventions" documentation [1], size calculations (especially > multiplication) should not be performed in memory allocator (or similar) > function arguments due to the risk of them overflowing. This could lead > to values wrapping around and a smaller allocation being made than the > caller was expecting. Using those allocations could lead to linear > overflows of heap memory and other misbehaviors. > > So, switch to flexible array member in the struct attribute_set_obj and > refactor the code accordingly to use the struct_size() helper instead of > the argument "size + count * size" in the kzalloc() function. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > Signed-off-by: Len Baker <len.baker@gmx.com> > --- > drivers/platform/x86/thinkpad_acpi.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 50ff04c84650..ed0b01ead796 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -1008,7 +1008,7 @@ struct attribute_set { > > struct attribute_set_obj { > struct attribute_set s; > - struct attribute *a; > + struct attribute *a[]; > } __attribute__((packed)); Whoa. I have so many questions... :) > > static struct attribute_set *create_attr_set(unsigned int max_members, > @@ -1020,13 +1020,11 @@ static struct attribute_set *create_attr_set(unsigned int max_members, > return NULL; > > /* Allocates space for implicit NULL at the end too */ > - sobj = kzalloc(sizeof(struct attribute_set_obj) + > - max_members * sizeof(struct attribute *), > - GFP_KERNEL); > + sobj = kzalloc(struct_size(sobj, a, max_members + 1), GFP_KERNEL); Whoa, this needs a lot more detail in the changelog if this is actually correct. The original code doesn't seem to match the comment? (Where is the +1?) So is this also a bug-fix? (I see the caller uses +2? Why? It seems to be using each of hotkey_attributes, plus 1 more attr, plus a final NULL?) > if (!sobj) > return NULL; > sobj->s.max_members = max_members; > - sobj->s.group.attrs = &sobj->a; > + sobj->s.group.attrs = sobj->a; > sobj->s.group.name = name; The caller also never sets a name? Why is struct attribute_set_obj marked as __packed? > > return &sobj->s; > -- > 2.25.1 >
Hi, On 9/20/21 7:58 AM, Kees Cook wrote: > On Sat, Sep 18, 2021 at 05:05:00PM +0200, Len Baker wrote: >> As noted in the "Deprecated Interfaces, Language Features, Attributes, >> and Conventions" documentation [1], size calculations (especially >> multiplication) should not be performed in memory allocator (or similar) >> function arguments due to the risk of them overflowing. This could lead >> to values wrapping around and a smaller allocation being made than the >> caller was expecting. Using those allocations could lead to linear >> overflows of heap memory and other misbehaviors. >> >> So, switch to flexible array member in the struct attribute_set_obj and >> refactor the code accordingly to use the struct_size() helper instead of >> the argument "size + count * size" in the kzalloc() function. >> >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments >> >> Signed-off-by: Len Baker <len.baker@gmx.com> >> --- >> drivers/platform/x86/thinkpad_acpi.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index 50ff04c84650..ed0b01ead796 100644 >> --- a/drivers/platform/x86/thinkpad_acpi.c >> +++ b/drivers/platform/x86/thinkpad_acpi.c >> @@ -1008,7 +1008,7 @@ struct attribute_set { >> >> struct attribute_set_obj { >> struct attribute_set s; >> - struct attribute *a; >> + struct attribute *a[]; >> } __attribute__((packed)); > > Whoa. I have so many questions... :) > >> >> static struct attribute_set *create_attr_set(unsigned int max_members, >> @@ -1020,13 +1020,11 @@ static struct attribute_set *create_attr_set(unsigned int max_members, >> return NULL; >> >> /* Allocates space for implicit NULL at the end too */ >> - sobj = kzalloc(sizeof(struct attribute_set_obj) + >> - max_members * sizeof(struct attribute *), >> - GFP_KERNEL); >> + sobj = kzalloc(struct_size(sobj, a, max_members + 1), GFP_KERNEL); > > Whoa, this needs a lot more detail in the changelog if this is actually > correct. The original code doesn't seem to match the comment? (Where is > the +1?) So is this also a bug-fix? Kees, at first I thought you were spot-on with this comment, but the truth is more subtle. struct attribute_set_obj was: struct attribute_set_obj { struct attribute_set s; struct attribute *a; } __attribute__((packed)); Another way of looking at this, which makes things more clear is as: struct attribute_set_obj { struct attribute_set s; struct attribute *a[1]; } __attribute__((packed)); So the sizeof(struct attribute_set_obj) in the original kzalloc call included room for 1 "extra" pointer which is reserved for the terminating NULL pointer. Changing the struct to: struct attribute_set_obj { struct attribute_set s; struct attribute *a[]; } __attribute__((packed)); Is equivalent to changing it to: struct attribute_set_obj { struct attribute_set s; struct attribute *a[0]; } __attribute__((packed)); So the change in the struct declaration reduces the sizeof(struct attribute_set_obj) by the size of 1 pointer, making the +1 necessary. So AFAICT there is actually no functional change here. Still I will hold off merging this until we agree on this :) > (I see the caller uses +2? Why? It seems to be using each of hotkey_attributes, > plus 1 more attr, plus a final NULL?) The +2 is actually for 2 extra attributes (making the total number of extra attributes +3 because the sizeof(struct attribute_set_obj) already includes 1 extra). FWIW these 2 extra attributes are for devices with a a physical rfkill on/off switch and for the device being a convertible capable of reporting laptop- vs tablet-mode. >> if (!sobj) >> return NULL; >> sobj->s.max_members = max_members; >> - sobj->s.group.attrs = &sobj->a; >> + sobj->s.group.attrs = sobj->a; >> sobj->s.group.name = name; > > The caller also never sets a name? attribute_group.name may be NULL, I don't know of (m)any drivers which actual set this to non NULL. > Why is struct attribute_set_obj marked as __packed? I have no clue, this seems completely unnecessary. Len Baker can you submit a separate patch removing the useless __packed ? Regards, Hans
On Tue, Sep 21, 2021 at 03:46:23PM +0200, Hans de Goede wrote: > Hi, > > On 9/20/21 7:58 AM, Kees Cook wrote: > > On Sat, Sep 18, 2021 at 05:05:00PM +0200, Len Baker wrote: > >> As noted in the "Deprecated Interfaces, Language Features, Attributes, > >> and Conventions" documentation [1], size calculations (especially > >> multiplication) should not be performed in memory allocator (or similar) > >> function arguments due to the risk of them overflowing. This could lead > >> to values wrapping around and a smaller allocation being made than the > >> caller was expecting. Using those allocations could lead to linear > >> overflows of heap memory and other misbehaviors. > >> > >> So, switch to flexible array member in the struct attribute_set_obj and > >> refactor the code accordingly to use the struct_size() helper instead of > >> the argument "size + count * size" in the kzalloc() function. > >> > >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > >> > >> Signed-off-by: Len Baker <len.baker@gmx.com> > >> --- > >> drivers/platform/x86/thinkpad_acpi.c | 8 +++----- > >> 1 file changed, 3 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > >> index 50ff04c84650..ed0b01ead796 100644 > >> --- a/drivers/platform/x86/thinkpad_acpi.c > >> +++ b/drivers/platform/x86/thinkpad_acpi.c > >> @@ -1008,7 +1008,7 @@ struct attribute_set { > >> > >> struct attribute_set_obj { > >> struct attribute_set s; > >> - struct attribute *a; > >> + struct attribute *a[]; > >> } __attribute__((packed)); > > > > Whoa. I have so many questions... :) > > > >> > >> static struct attribute_set *create_attr_set(unsigned int max_members, > >> @@ -1020,13 +1020,11 @@ static struct attribute_set *create_attr_set(unsigned int max_members, > >> return NULL; > >> > >> /* Allocates space for implicit NULL at the end too */ > >> - sobj = kzalloc(sizeof(struct attribute_set_obj) + > >> - max_members * sizeof(struct attribute *), > >> - GFP_KERNEL); > >> + sobj = kzalloc(struct_size(sobj, a, max_members + 1), GFP_KERNEL); > > > > Whoa, this needs a lot more detail in the changelog if this is actually > > correct. The original code doesn't seem to match the comment? (Where is > > the +1?) So is this also a bug-fix? > > Kees, at first I thought you were spot-on with this comment, but the > truth is more subtle. struct attribute_set_obj was: > > struct attribute_set_obj { > struct attribute_set s; > struct attribute *a; > } __attribute__((packed)); > > Another way of looking at this, which makes things more clear is as: > > struct attribute_set_obj { > struct attribute_set s; > struct attribute *a[1]; > } __attribute__((packed)); > > So the sizeof(struct attribute_set_obj) in the original kzalloc call > included room for 1 "extra" pointer which is reserved for the terminating > NULL pointer. > > Changing the struct to: > > struct attribute_set_obj { > struct attribute_set s; > struct attribute *a[]; > } __attribute__((packed)); > > Is equivalent to changing it to: > > struct attribute_set_obj { > struct attribute_set s; > struct attribute *a[0]; > } __attribute__((packed)); > > So the change in the struct declaration reduces the sizeof(struct attribute_set_obj) > by the size of 1 pointer, making the +1 necessary. > > So AFAICT there is actually no functional change here. > > Still I will hold off merging this until we agree on this :) First off, why is a single driver doing so many odd things with attribute groups? Why not just use them the way that the rest of the kernel does? Why does this driver need this special handling and no one else does? I think the default way of handling if an attribute is enabled or not, should suffice here, and make things much simpler overall as all of this crazy attribute handling can just be removed. Bonus would also be that I think it would fix the race conditions that happen when trying to create attributes after the device is bound to the driver that I think the existing driver has today. > > (I see the caller uses +2? Why? It seems to be using each of hotkey_attributes, > > plus 1 more attr, plus a final NULL?) > > The +2 is actually for 2 extra attributes (making the total number > of extra attributes +3 because the sizeof(struct attribute_set_obj) > already includes 1 extra). > > FWIW these 2 extra attributes are for devices with a > a physical rfkill on/off switch and for the device being > a convertible capable of reporting laptop- vs tablet-mode. Again, using the default way to show (or not show) attributes should solve this issue. Why not just use that instead? > >> if (!sobj) > >> return NULL; > >> sobj->s.max_members = max_members; > >> - sobj->s.group.attrs = &sobj->a; > >> + sobj->s.group.attrs = sobj->a; > >> sobj->s.group.name = name; > > > > The caller also never sets a name? > > attribute_group.name may be NULL, I don't know > of (m)any drivers which actual set this to non NULL. It is used by some, that is how you can put all of the attributes in a subdirectory automatically. No idea if that's needed here... All attributes for this driver are documented in Documentation/ABI/, right? :) thanks, greg k-h
Hi, On 9/21/21 5:15 PM, Greg KH wrote: > On Tue, Sep 21, 2021 at 03:46:23PM +0200, Hans de Goede wrote: >> Hi, >> >> On 9/20/21 7:58 AM, Kees Cook wrote: >>> On Sat, Sep 18, 2021 at 05:05:00PM +0200, Len Baker wrote: >>>> As noted in the "Deprecated Interfaces, Language Features, Attributes, >>>> and Conventions" documentation [1], size calculations (especially >>>> multiplication) should not be performed in memory allocator (or similar) >>>> function arguments due to the risk of them overflowing. This could lead >>>> to values wrapping around and a smaller allocation being made than the >>>> caller was expecting. Using those allocations could lead to linear >>>> overflows of heap memory and other misbehaviors. >>>> >>>> So, switch to flexible array member in the struct attribute_set_obj and >>>> refactor the code accordingly to use the struct_size() helper instead of >>>> the argument "size + count * size" in the kzalloc() function. >>>> >>>> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments >>>> >>>> Signed-off-by: Len Baker <len.baker@gmx.com> >>>> --- >>>> drivers/platform/x86/thinkpad_acpi.c | 8 +++----- >>>> 1 file changed, 3 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >>>> index 50ff04c84650..ed0b01ead796 100644 >>>> --- a/drivers/platform/x86/thinkpad_acpi.c >>>> +++ b/drivers/platform/x86/thinkpad_acpi.c >>>> @@ -1008,7 +1008,7 @@ struct attribute_set { >>>> >>>> struct attribute_set_obj { >>>> struct attribute_set s; >>>> - struct attribute *a; >>>> + struct attribute *a[]; >>>> } __attribute__((packed)); >>> >>> Whoa. I have so many questions... :) >>> >>>> >>>> static struct attribute_set *create_attr_set(unsigned int max_members, >>>> @@ -1020,13 +1020,11 @@ static struct attribute_set *create_attr_set(unsigned int max_members, >>>> return NULL; >>>> >>>> /* Allocates space for implicit NULL at the end too */ >>>> - sobj = kzalloc(sizeof(struct attribute_set_obj) + >>>> - max_members * sizeof(struct attribute *), >>>> - GFP_KERNEL); >>>> + sobj = kzalloc(struct_size(sobj, a, max_members + 1), GFP_KERNEL); >>> >>> Whoa, this needs a lot more detail in the changelog if this is actually >>> correct. The original code doesn't seem to match the comment? (Where is >>> the +1?) So is this also a bug-fix? >> >> Kees, at first I thought you were spot-on with this comment, but the >> truth is more subtle. struct attribute_set_obj was: >> >> struct attribute_set_obj { >> struct attribute_set s; >> struct attribute *a; >> } __attribute__((packed)); >> >> Another way of looking at this, which makes things more clear is as: >> >> struct attribute_set_obj { >> struct attribute_set s; >> struct attribute *a[1]; >> } __attribute__((packed)); >> >> So the sizeof(struct attribute_set_obj) in the original kzalloc call >> included room for 1 "extra" pointer which is reserved for the terminating >> NULL pointer. >> >> Changing the struct to: >> >> struct attribute_set_obj { >> struct attribute_set s; >> struct attribute *a[]; >> } __attribute__((packed)); >> >> Is equivalent to changing it to: >> >> struct attribute_set_obj { >> struct attribute_set s; >> struct attribute *a[0]; >> } __attribute__((packed)); >> >> So the change in the struct declaration reduces the sizeof(struct attribute_set_obj) >> by the size of 1 pointer, making the +1 necessary. >> >> So AFAICT there is actually no functional change here. >> >> Still I will hold off merging this until we agree on this :) > > First off, why is a single driver doing so many odd things with > attribute groups? Why not just use them the way that the rest of the > kernel does? Why does this driver need this special handling and no one > else does? The thinkpad_acpi driver carries a lot of legacy with it. So in general we are careful with making changes because some people still use quite old ThinkPad-s and it is tricky to make sure we don't break stuff on older models. So yeah there is some cruft in a bunch of places in this driver. In this case it seems that cleaning things up is a straight forward fix though, so we really should do so. > > I think the default way of handling if an attribute is enabled or not, > should suffice here, and make things much simpler overall as all of this > crazy attribute handling can just be removed. > > Bonus would also be that I think it would fix the race conditions that > happen when trying to create attributes after the device is bound to the > driver that I think the existing driver has today. > >>> (I see the caller uses +2? Why? It seems to be using each of hotkey_attributes, >>> plus 1 more attr, plus a final NULL?) >> >> The +2 is actually for 2 extra attributes (making the total number >> of extra attributes +3 because the sizeof(struct attribute_set_obj) >> already includes 1 extra). >> >> FWIW these 2 extra attributes are for devices with a >> a physical rfkill on/off switch and for the device being >> a convertible capable of reporting laptop- vs tablet-mode. > > Again, using the default way to show (or not show) attributes should > solve this issue. Why not just use that instead? I agree, moving to a "fixed" attribute_group, with an is_visible check for the optional attributes would be a much better fix and allow removal of a whole bunch of custom code. If anyone following this thread could submit a patch doing that, then that would be great. >>>> if (!sobj) >>>> return NULL; >>>> sobj->s.max_members = max_members; >>>> - sobj->s.group.attrs = &sobj->a; >>>> + sobj->s.group.attrs = sobj->a; >>>> sobj->s.group.name = name; >>> >>> The caller also never sets a name? >> >> attribute_group.name may be NULL, I don't know >> of (m)any drivers which actual set this to non NULL. > > It is used by some, that is how you can put all of the attributes in a > subdirectory automatically. No idea if that's needed here... > > All attributes for this driver are documented in Documentation/ABI/, > right? :) I'm not sure if all attributes are documented, but a lot of them (including all recently added ones) are documented in: Documentation/admin-guide/laptops/thinkpad-acpi.rst Regards, Hans
On Tue, Sep 21, 2021 at 05:38:39PM +0200, Hans de Goede wrote: > > All attributes for this driver are documented in Documentation/ABI/, > > right? :) > > I'm not sure if all attributes are documented, but a lot of them > (including all recently added ones) are documented in: > Documentation/admin-guide/laptops/thinkpad-acpi.rst They should also go into Documentation/ABI/ which is where sysfs files are documented. We are working on tools that make parsing that easier, so it would be good to keep them out of other random documentation files whenever possible. thanks, greg k-h
Hi, On Tue, Sep 21, 2021 at 03:46:23PM +0200, Hans de Goede wrote: > On 9/20/21 7:58 AM, Kees Cook wrote: > > On Sat, Sep 18, 2021 at 05:05:00PM +0200, Len Baker wrote: > >> > >> static struct attribute_set *create_attr_set(unsigned int max_members, > >> @@ -1020,13 +1020,11 @@ static struct attribute_set *create_attr_set(unsigned int max_members, > >> return NULL; > >> > >> /* Allocates space for implicit NULL at the end too */ > >> - sobj = kzalloc(sizeof(struct attribute_set_obj) + > >> - max_members * sizeof(struct attribute *), > >> - GFP_KERNEL); > >> + sobj = kzalloc(struct_size(sobj, a, max_members + 1), GFP_KERNEL); > > > > Whoa, this needs a lot more detail in the changelog if this is actually > > correct. The original code doesn't seem to match the comment? (Where is > > the +1?) So is this also a bug-fix? > > Kees, at first I thought you were spot-on with this comment, but the > truth is more subtle. struct attribute_set_obj was: > > struct attribute_set_obj { > struct attribute_set s; > struct attribute *a; > } __attribute__((packed)); > > Another way of looking at this, which makes things more clear is as: > > struct attribute_set_obj { > struct attribute_set s; > struct attribute *a[1]; > } __attribute__((packed)); > > So the sizeof(struct attribute_set_obj) in the original kzalloc call > included room for 1 "extra" pointer which is reserved for the terminating > NULL pointer. > > Changing the struct to: > > struct attribute_set_obj { > struct attribute_set s; > struct attribute *a[]; > } __attribute__((packed)); > > Is equivalent to changing it to: > > struct attribute_set_obj { > struct attribute_set s; > struct attribute *a[0]; > } __attribute__((packed)); > > So the change in the struct declaration reduces the sizeof(struct attribute_set_obj) > by the size of 1 pointer, making the +1 necessary. > > So AFAICT there is actually no functional change here. Hans, thanks for the explanation. Yes, this is the reason I added the "plus 1". Not only based on the comment :) Regards, Len
Hi, On Tue, Sep 21, 2021 at 05:15:35PM +0200, Greg KH wrote: > > First off, why is a single driver doing so many odd things with > attribute groups? Why not just use them the way that the rest of the > kernel does? Why does this driver need this special handling and no one > else does? Is [1] the correct way to deal with devices attributes? I think so. [1] https://www.kernel.org/doc/html/latest/driver-api/driver-model/driver.html#attributes > > I think the default way of handling if an attribute is enabled or not, > should suffice here, and make things much simpler overall as all of this > crazy attribute handling can just be removed. Sorry but what is the default way? Would it be correct to check if the file exists? > > Bonus would also be that I think it would fix the race conditions that > happen when trying to create attributes after the device is bound to the > driver that I think the existing driver has today. > > > > (I see the caller uses +2? Why? It seems to be using each of hotkey_attributes, > > > plus 1 more attr, plus a final NULL?) > > > > The +2 is actually for 2 extra attributes (making the total number > > of extra attributes +3 because the sizeof(struct attribute_set_obj) > > already includes 1 extra). > > > > FWIW these 2 extra attributes are for devices with a > > a physical rfkill on/off switch and for the device being > > a convertible capable of reporting laptop- vs tablet-mode. > > Again, using the default way to show (or not show) attributes should > solve this issue. Why not just use that instead? What is the default way? Would it be correct to use device_create_file() and device_remove_file()? Sorry if it is a trivial question but I am a kernel newbie :) I have a lot to learn. Any suggestion or a good driver to look at would be greatly appreciated. Thanks, Len
On Sat, Sep 25, 2021 at 12:40:44PM +0200, Len Baker wrote: > Hi, > > On Tue, Sep 21, 2021 at 05:15:35PM +0200, Greg KH wrote: > > > > First off, why is a single driver doing so many odd things with > > attribute groups? Why not just use them the way that the rest of the > > kernel does? Why does this driver need this special handling and no one > > else does? > > Is [1] the correct way to deal with devices attributes? I think so. > > [1] https://www.kernel.org/doc/html/latest/driver-api/driver-model/driver.html#attributes No, do not use driver_create_file(), see: http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/ as a more up to date thing. Someone should fix that in-kernel documentation one day :) > > I think the default way of handling if an attribute is enabled or not, > > should suffice here, and make things much simpler overall as all of this > > crazy attribute handling can just be removed. > > Sorry but what is the default way? Would it be correct to check if the > file exists? Use the is_visable() callback for the attribute group to enable/disable the creation of the sysfs file. > > Bonus would also be that I think it would fix the race conditions that > > happen when trying to create attributes after the device is bound to the > > driver that I think the existing driver has today. > > > > > > (I see the caller uses +2? Why? It seems to be using each of hotkey_attributes, > > > > plus 1 more attr, plus a final NULL?) > > > > > > The +2 is actually for 2 extra attributes (making the total number > > > of extra attributes +3 because the sizeof(struct attribute_set_obj) > > > already includes 1 extra). > > > > > > FWIW these 2 extra attributes are for devices with a > > > a physical rfkill on/off switch and for the device being > > > a convertible capable of reporting laptop- vs tablet-mode. > > > > Again, using the default way to show (or not show) attributes should > > solve this issue. Why not just use that instead? > > What is the default way? Would it be correct to use device_create_file() > and device_remove_file()? Put all the attributes into an attribute group and attach it to the driver. The driver core will create/remove the files when needed. The link above should help explain that a bit better, and I can point you at examples if needed. Does that help? thanks, greg k-h
Hi Greg, On Sat, Sep 25, 2021 at 01:07:20PM +0200, Greg KH wrote: > On Sat, Sep 25, 2021 at 12:40:44PM +0200, Len Baker wrote: > > Hi, > > > > On Tue, Sep 21, 2021 at 05:15:35PM +0200, Greg KH wrote: > > > > > > First off, why is a single driver doing so many odd things with > > > attribute groups? Why not just use them the way that the rest of the > > > kernel does? Why does this driver need this special handling and no one > > > else does? > > > > Is [1] the correct way to deal with devices attributes? I think so. > > > > [1] https://www.kernel.org/doc/html/latest/driver-api/driver-model/driver.html#attributes > > No, do not use driver_create_file(), see: > http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/ > as a more up to date thing. Ok, understood. Thanks. > > Someone should fix that in-kernel documentation one day :) > > > > I think the default way of handling if an attribute is enabled or not, > > > should suffice here, and make things much simpler overall as all of this > > > crazy attribute handling can just be removed. > > > > Sorry but what is the default way? Would it be correct to check if the > > file exists? > > Use the is_visable() callback for the attribute group to enable/disable > the creation of the sysfs file. Ok, I will take a look at it. > > > > Bonus would also be that I think it would fix the race conditions that > > > happen when trying to create attributes after the device is bound to the > > > driver that I think the existing driver has today. > > > > > > > > (I see the caller uses +2? Why? It seems to be using each of hotkey_attributes, > > > > > plus 1 more attr, plus a final NULL?) > > > > > > > > The +2 is actually for 2 extra attributes (making the total number > > > > of extra attributes +3 because the sizeof(struct attribute_set_obj) > > > > already includes 1 extra). > > > > > > > > FWIW these 2 extra attributes are for devices with a > > > > a physical rfkill on/off switch and for the device being > > > > a convertible capable of reporting laptop- vs tablet-mode. > > > > > > Again, using the default way to show (or not show) attributes should > > > solve this issue. Why not just use that instead? > > > > What is the default way? Would it be correct to use device_create_file() > > and device_remove_file()? > > Put all the attributes into an attribute group and attach it to the > driver. The driver core will create/remove the files when needed. The > link above should help explain that a bit better, and I can point you at > examples if needed. > > Does that help? Yes, things are clearer to me now. Also, since the only way to learn is to do so, I will take the task to switch this driver to the common use of attributes. Thank you very much for your time and guidance. Regards, Len
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 50ff04c84650..ed0b01ead796 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -1008,7 +1008,7 @@ struct attribute_set { struct attribute_set_obj { struct attribute_set s; - struct attribute *a; + struct attribute *a[]; } __attribute__((packed)); static struct attribute_set *create_attr_set(unsigned int max_members, @@ -1020,13 +1020,11 @@ static struct attribute_set *create_attr_set(unsigned int max_members, return NULL; /* Allocates space for implicit NULL at the end too */ - sobj = kzalloc(sizeof(struct attribute_set_obj) + - max_members * sizeof(struct attribute *), - GFP_KERNEL); + sobj = kzalloc(struct_size(sobj, a, max_members + 1), GFP_KERNEL); if (!sobj) return NULL; sobj->s.max_members = max_members; - sobj->s.group.attrs = &sobj->a; + sobj->s.group.attrs = sobj->a; sobj->s.group.name = name; return &sobj->s;
As noted in the "Deprecated Interfaces, Language Features, Attributes, and Conventions" documentation [1], size calculations (especially multiplication) should not be performed in memory allocator (or similar) function arguments due to the risk of them overflowing. This could lead to values wrapping around and a smaller allocation being made than the caller was expecting. Using those allocations could lead to linear overflows of heap memory and other misbehaviors. So, switch to flexible array member in the struct attribute_set_obj and refactor the code accordingly to use the struct_size() helper instead of the argument "size + count * size" in the kzalloc() function. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments Signed-off-by: Len Baker <len.baker@gmx.com> --- drivers/platform/x86/thinkpad_acpi.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) -- 2.25.1