diff mbox series

platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic

Message ID 20210918150500.21530-1-len.baker@gmx.com (mailing list archive)
State Changes Requested
Headers show
Series platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic | expand

Commit Message

Len Baker Sept. 18, 2021, 3:05 p.m. UTC
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

Comments

Kees Cook Sept. 20, 2021, 5:58 a.m. UTC | #1
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
>
Hans de Goede Sept. 21, 2021, 1:46 p.m. UTC | #2
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
Greg KH Sept. 21, 2021, 3:15 p.m. UTC | #3
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
Hans de Goede Sept. 21, 2021, 3:38 p.m. UTC | #4
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
Greg KH Sept. 21, 2021, 3:45 p.m. UTC | #5
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
Len Baker Sept. 25, 2021, 10:37 a.m. UTC | #6
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
Len Baker Sept. 25, 2021, 10:40 a.m. UTC | #7
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
Greg KH Sept. 25, 2021, 11:07 a.m. UTC | #8
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
Len Baker Sept. 25, 2021, 1:33 p.m. UTC | #9
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 mbox series

Patch

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;