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,
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.
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.
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
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.
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
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.
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.
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.
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.
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
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
linux-stable-mirror@lists.linaro.org
-
Dmitry Torokhov -
Greg Kroah-Hartman -
Zijun Hu