driver core: Fix an uninitialized variable is used by __device_attach()

[PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Zijun Hu 1 year, 5 months ago

From: Zijun Hu <quic_zijuhu@quicinc.com>

An uninitialized variable @data.have_async may be used as analyzed
by the following inline comments:

static int __device_attach(struct device *dev, bool allow_async)
{
	// if @allow_async is true.

	...
	struct device_attach_data data = {
		.dev = dev,
		.check_async = allow_async,
		.want_async = false,
	};
	// @data.have_async is not initialized.

	...
	ret = bus_for_each_drv(dev->bus, NULL, &data,
			__device_attach_driver);
	// @data.have_async must not be set by __device_attach_driver() if
 	// @dev->bus does not have driver which allows probe asynchronously

	if (!ret && allow_async && data.have_async) {
	// Above @data.have_async is not uninitialized but used.
		...
	}
	...
}

It may be unnecessary to trigger the second pass probing asynchronous
drivers for the device @dev.

Fixed by initializing @data.have_async to false.

Fixes: 765230b5f084 ("driver-core: add asynchronous probing support for drivers")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9b745ba54de1..b0c44b0846aa 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1021,6 +1021,7 @@ static int __device_attach(struct device *dev, bool allow_async)
 			.dev = dev,
 			.check_async = allow_async,
 			.want_async = false,
+			.have_async = false,
 		};
 
 		if (dev->parent)

---
base-commit: 87ee9981d1f86ee9b1623a46c7f9e4ac24461fe4
change-id: 20240823-fix_have_async-3a135618d91b

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Greg Kroah-Hartman 1 year, 5 months ago

On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> An uninitialized variable @data.have_async may be used as analyzed
> by the following inline comments:
> 
> static int __device_attach(struct device *dev, bool allow_async)
> {
> 	// if @allow_async is true.
> 
> 	...
> 	struct device_attach_data data = {
> 		.dev = dev,
> 		.check_async = allow_async,
> 		.want_async = false,
> 	};
> 	// @data.have_async is not initialized.

As Dmitry said, this is incorrect, please fix your broken code analysis
tool, it is obviously not working properly :(

thanks,

greg k-h

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Zijun Hu 1 year, 5 months ago

On 2024/8/23 08:14, Greg Kroah-Hartman wrote:
> On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> An uninitialized variable @data.have_async may be used as analyzed
>> by the following inline comments:
>>
>> static int __device_attach(struct device *dev, bool allow_async)
>> {
>> 	// if @allow_async is true.
>>
>> 	...
>> 	struct device_attach_data data = {
>> 		.dev = dev,
>> 		.check_async = allow_async,
>> 		.want_async = false,
>> 	};
>> 	// @data.have_async is not initialized.
> 
> As Dmitry said, this is incorrect, please fix your broken code analysis
> tool, it is obviously not working properly :(
> 

let us slow down firstly to confirm if what Dmitry said is right firstly.

it is not related to any analysis tool, i notice it by reading code.

> thanks,
> 
> greg k-h

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Dmitry Torokhov 1 year, 5 months ago

Hi,

On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> An uninitialized variable @data.have_async may be used as analyzed
> by the following inline comments:
> 
> static int __device_attach(struct device *dev, bool allow_async)
> {
> 	// if @allow_async is true.
> 
> 	...
> 	struct device_attach_data data = {
> 		.dev = dev,
> 		.check_async = allow_async,
> 		.want_async = false,
> 	};
> 	// @data.have_async is not initialized.

No, in the presence of a structure initializer fields not explicitly
initialized will be set to 0 by the compiler.

There is no issue here.

Thanks.

-- 
Dmitry

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Zijun Hu 1 year, 5 months ago

On 2024/8/23 08:02, Dmitry Torokhov wrote:
> Hi,
> 
> On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> An uninitialized variable @data.have_async may be used as analyzed
>> by the following inline comments:
>>
>> static int __device_attach(struct device *dev, bool allow_async)
>> {
>> 	// if @allow_async is true.
>>
>> 	...
>> 	struct device_attach_data data = {
>> 		.dev = dev,
>> 		.check_async = allow_async,
>> 		.want_async = false,
>> 	};
>> 	// @data.have_async is not initialized.
> 
> No, in the presence of a structure initializer fields not explicitly
> initialized will be set to 0 by the compiler.
> 
yes. you are right. compiler will implicitly initialize @data.have_async.

is it worthy to explicitly initialize @data.have_async as existing
@data.want_async as well to prevent misleading human readers since this
initialization approach appears to partial initialization ?

> There is no issue here.
> 
> Thanks.
>

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Zijun Hu 1 year, 5 months ago

On 2024/8/23 08:02, Dmitry Torokhov wrote:
> Hi,
> 
> On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> An uninitialized variable @data.have_async may be used as analyzed
>> by the following inline comments:
>>
>> static int __device_attach(struct device *dev, bool allow_async)
>> {
>> 	// if @allow_async is true.
>>
>> 	...
>> 	struct device_attach_data data = {
>> 		.dev = dev,
>> 		.check_async = allow_async,
>> 		.want_async = false,
>> 	};
>> 	// @data.have_async is not initialized.
> 
> No, in the presence of a structure initializer fields not explicitly
> initialized will be set to 0 by the compiler.
> 
really?
do all C compilers have such behavior ?

> There is no issue here.
> 
> Thanks.
>

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Dmitry Torokhov 1 year, 5 months ago

On Fri, Aug 23, 2024 at 08:46:12AM +0800, Zijun Hu wrote:
> On 2024/8/23 08:02, Dmitry Torokhov wrote:
> > Hi,
> > 
> > On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> An uninitialized variable @data.have_async may be used as analyzed
> >> by the following inline comments:
> >>
> >> static int __device_attach(struct device *dev, bool allow_async)
> >> {
> >> 	// if @allow_async is true.
> >>
> >> 	...
> >> 	struct device_attach_data data = {
> >> 		.dev = dev,
> >> 		.check_async = allow_async,
> >> 		.want_async = false,
> >> 	};
> >> 	// @data.have_async is not initialized.
> > 
> > No, in the presence of a structure initializer fields not explicitly
> > initialized will be set to 0 by the compiler.
> > 
> really?
> do all C compilers have such behavior ?

Yes, all conforming to the C standard.

Thanks.

-- 
Dmitry

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Greg Kroah-Hartman 1 year, 5 months ago

On Fri, Aug 23, 2024 at 08:46:12AM +0800, Zijun Hu wrote:
> On 2024/8/23 08:02, Dmitry Torokhov wrote:
> > Hi,
> > 
> > On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> An uninitialized variable @data.have_async may be used as analyzed
> >> by the following inline comments:
> >>
> >> static int __device_attach(struct device *dev, bool allow_async)
> >> {
> >> 	// if @allow_async is true.
> >>
> >> 	...
> >> 	struct device_attach_data data = {
> >> 		.dev = dev,
> >> 		.check_async = allow_async,
> >> 		.want_async = false,
> >> 	};
> >> 	// @data.have_async is not initialized.
> > 
> > No, in the presence of a structure initializer fields not explicitly
> > initialized will be set to 0 by the compiler.
> > 
> really?
> do all C compilers have such behavior ?

Oh wait, if this were static, then yes, it would all be set to 0, sorry,
I misread this.

This is on the stack so it needs to be zeroed out explicitly.  We should
set the whole thing to 0 and then set only the fields we want to
override to ensure it's all correct.

thanks,

greg k-h

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Dmitry Torokhov 1 year, 5 months ago

On Fri, Aug 23, 2024 at 09:14:12AM +0800, Greg Kroah-Hartman wrote:
> On Fri, Aug 23, 2024 at 08:46:12AM +0800, Zijun Hu wrote:
> > On 2024/8/23 08:02, Dmitry Torokhov wrote:
> > > Hi,
> > > 
> > > On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
> > >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> > >>
> > >> An uninitialized variable @data.have_async may be used as analyzed
> > >> by the following inline comments:
> > >>
> > >> static int __device_attach(struct device *dev, bool allow_async)
> > >> {
> > >> 	// if @allow_async is true.
> > >>
> > >> 	...
> > >> 	struct device_attach_data data = {
> > >> 		.dev = dev,
> > >> 		.check_async = allow_async,
> > >> 		.want_async = false,
> > >> 	};
> > >> 	// @data.have_async is not initialized.
> > > 
> > > No, in the presence of a structure initializer fields not explicitly
> > > initialized will be set to 0 by the compiler.
> > > 
> > really?
> > do all C compilers have such behavior ?
> 
> Oh wait, if this were static, then yes, it would all be set to 0, sorry,
> I misread this.
> 
> This is on the stack so it needs to be zeroed out explicitly.  We should
> set the whole thing to 0 and then set only the fields we want to
> override to ensure it's all correct.

No we do not. ISO/IEC 9899:201x 6.7.9 Initialization:

"21 If there are fewer initializers in a brace-enclosed list than there
are elements or members of an aggregate, or fewer characters in a string
literal used to initialize an array of known size than there are
elements in the array, the remainder of the aggregate shall be
initialized implicitly the same as objects that have static storage
duration."

That is why you can 0-initialize a structure by doing:

	struct s s1 = { 0 };

or even

	struct s s1 = { };

Thanks.

-- 
Dmitry

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Zijun Hu 1 year, 5 months ago

On 2024/8/23 09:25, Dmitry Torokhov wrote:
> On Fri, Aug 23, 2024 at 09:14:12AM +0800, Greg Kroah-Hartman wrote:
>> On Fri, Aug 23, 2024 at 08:46:12AM +0800, Zijun Hu wrote:
>>> On 2024/8/23 08:02, Dmitry Torokhov wrote:
>>>> Hi,
>>>>
>>>> On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
>>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>
>>>>> An uninitialized variable @data.have_async may be used as analyzed
>>>>> by the following inline comments:
>>>>>
>>>>> static int __device_attach(struct device *dev, bool allow_async)
>>>>> {
>>>>> 	// if @allow_async is true.
>>>>>
>>>>> 	...
>>>>> 	struct device_attach_data data = {
>>>>> 		.dev = dev,
>>>>> 		.check_async = allow_async,
>>>>> 		.want_async = false,
>>>>> 	};
>>>>> 	// @data.have_async is not initialized.
>>>>
>>>> No, in the presence of a structure initializer fields not explicitly
>>>> initialized will be set to 0 by the compiler.
>>>>
>>> really?
>>> do all C compilers have such behavior ?
>>
>> Oh wait, if this were static, then yes, it would all be set to 0, sorry,
>> I misread this.
>>
>> This is on the stack so it needs to be zeroed out explicitly.  We should
>> set the whole thing to 0 and then set only the fields we want to
>> override to ensure it's all correct.
> 
> No we do not. ISO/IEC 9899:201x 6.7.9 Initialization:
> 
> "21 If there are fewer initializers in a brace-enclosed list than there
> are elements or members of an aggregate, or fewer characters in a string
> literal used to initialize an array of known size than there are
> elements in the array, the remainder of the aggregate shall be
> initialized implicitly the same as objects that have static storage
> duration."
> 
> That is why you can 0-initialize a structure by doing:
> 
> 	struct s s1 = { 0 };
> 
> or even
> 
> 	struct s s1 = { };
> 
For above both initialization: it appears to initialize the whole struct.
but For the initialization approach we discuss, it appears to
initialize partial struct, it is easy to mislead developers.

> Thanks.
>

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Greg Kroah-Hartman 1 year, 5 months ago

On Thu, Aug 22, 2024 at 06:25:15PM -0700, Dmitry Torokhov wrote:
> On Fri, Aug 23, 2024 at 09:14:12AM +0800, Greg Kroah-Hartman wrote:
> > On Fri, Aug 23, 2024 at 08:46:12AM +0800, Zijun Hu wrote:
> > > On 2024/8/23 08:02, Dmitry Torokhov wrote:
> > > > Hi,
> > > > 
> > > > On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
> > > >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> > > >>
> > > >> An uninitialized variable @data.have_async may be used as analyzed
> > > >> by the following inline comments:
> > > >>
> > > >> static int __device_attach(struct device *dev, bool allow_async)
> > > >> {
> > > >> 	// if @allow_async is true.
> > > >>
> > > >> 	...
> > > >> 	struct device_attach_data data = {
> > > >> 		.dev = dev,
> > > >> 		.check_async = allow_async,
> > > >> 		.want_async = false,
> > > >> 	};
> > > >> 	// @data.have_async is not initialized.
> > > > 
> > > > No, in the presence of a structure initializer fields not explicitly
> > > > initialized will be set to 0 by the compiler.
> > > > 
> > > really?
> > > do all C compilers have such behavior ?
> > 
> > Oh wait, if this were static, then yes, it would all be set to 0, sorry,
> > I misread this.
> > 
> > This is on the stack so it needs to be zeroed out explicitly.  We should
> > set the whole thing to 0 and then set only the fields we want to
> > override to ensure it's all correct.
> 
> No we do not. ISO/IEC 9899:201x 6.7.9 Initialization:
> 
> "21 If there are fewer initializers in a brace-enclosed list than there
> are elements or members of an aggregate, or fewer characters in a string
> literal used to initialize an array of known size than there are
> elements in the array, the remainder of the aggregate shall be
> initialized implicitly the same as objects that have static storage
> duration."
> 
> That is why you can 0-initialize a structure by doing:
> 
> 	struct s s1 = { 0 };
> 
> or even
> 
> 	struct s s1 = { };

{sigh}  I always get this wrong, also there's the question "are holes
in the structure also set to 0" which as you can see from the above
spec, should also be true.  But numerous places in the kernel explicitly
use memset() to "make sure" of that.

thanks,

greg k-h

Re: [PATCH] driver core: Fix an uninitialized variable is used by __device_attach()

Posted by Dmitry Torokhov 1 year, 5 months ago

On Fri, Aug 23, 2024 at 02:11:45PM +0800, Greg Kroah-Hartman wrote:
> On Thu, Aug 22, 2024 at 06:25:15PM -0700, Dmitry Torokhov wrote:
> > On Fri, Aug 23, 2024 at 09:14:12AM +0800, Greg Kroah-Hartman wrote:
> > > On Fri, Aug 23, 2024 at 08:46:12AM +0800, Zijun Hu wrote:
> > > > On 2024/8/23 08:02, Dmitry Torokhov wrote:
> > > > > Hi,
> > > > > 
> > > > > On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
> > > > >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> > > > >>
> > > > >> An uninitialized variable @data.have_async may be used as analyzed
> > > > >> by the following inline comments:
> > > > >>
> > > > >> static int __device_attach(struct device *dev, bool allow_async)
> > > > >> {
> > > > >> 	// if @allow_async is true.
> > > > >>
> > > > >> 	...
> > > > >> 	struct device_attach_data data = {
> > > > >> 		.dev = dev,
> > > > >> 		.check_async = allow_async,
> > > > >> 		.want_async = false,
> > > > >> 	};
> > > > >> 	// @data.have_async is not initialized.
> > > > > 
> > > > > No, in the presence of a structure initializer fields not explicitly
> > > > > initialized will be set to 0 by the compiler.
> > > > > 
> > > > really?
> > > > do all C compilers have such behavior ?
> > > 
> > > Oh wait, if this were static, then yes, it would all be set to 0, sorry,
> > > I misread this.
> > > 
> > > This is on the stack so it needs to be zeroed out explicitly.  We should
> > > set the whole thing to 0 and then set only the fields we want to
> > > override to ensure it's all correct.
> > 
> > No we do not. ISO/IEC 9899:201x 6.7.9 Initialization:
> > 
> > "21 If there are fewer initializers in a brace-enclosed list than there
> > are elements or members of an aggregate, or fewer characters in a string
> > literal used to initialize an array of known size than there are
> > elements in the array, the remainder of the aggregate shall be
> > initialized implicitly the same as objects that have static storage
> > duration."
> > 
> > That is why you can 0-initialize a structure by doing:
> > 
> > 	struct s s1 = { 0 };
> > 
> > or even
> > 
> > 	struct s s1 = { };
> 
> {sigh}  I always get this wrong, also there's the question "are holes
> in the structure also set to 0" which as you can see from the above
> spec, should also be true.  But numerous places in the kernel explicitly
> use memset() to "make sure" of that.

I think it has more to do with our preference for having declarations
before code, so if there is complex or conditional initialization then
it is more natural to declare uninitialized variable, and then later
explicitly memset() it and assign required values to members.

Thanks.

-- 
Dmitry