* Re: [PATCH] cxl: core: Initialize eiw and eig to fix potential uninitialized use
2025-04-19 20:35 [PATCH] cxl: core: Initialize eiw and eig to fix potential uninitialized use Purva Yeshi
@ 2025-04-20 10:43 ` Li Ming
2025-04-21 15:40 ` Gregory Price
2025-04-21 15:41 ` Gregory Price
2025-04-21 17:39 ` Dan Williams
2 siblings, 1 reply; 6+ messages in thread
From: Li Ming @ 2025-04-20 10:43 UTC (permalink / raw)
To: Purva Yeshi
Cc: huang.ying.caritas, linux-cxl, linux-kernel, dave,
jonathan.cameron, dave.jiang, alison.schofield, vishal.l.verma,
ira.weiny, dan.j.williams
On 4/20/2025 4:35 AM, Purva Yeshi wrote:
> Fix Smatch-detected issue:
>
> drivers/cxl/core/region.c:1292 check_interleave_cap()
> error: uninitialized symbol 'eiw'.
> drivers/cxl/core/region.c:1297 check_interleave_cap()
> error: uninitialized symbol 'eig'.
> drivers/cxl/core/region.c:1299 check_interleave_cap()
> error: uninitialized symbol 'eig'.
>
> Smatch reports possible uninitialized usage of these variables if
> the helper functions ways_to_eiw() or granularity_to_eig() fail to
> assign values under certain conditions.
>
> Initialize the eiw and eig variables to zero when they are declared
> in check_interleave_cap() to prevent potential use of uninitialized
> values.
I also met them during checking cxl drivers code with code inspection tools. But they are not real issues, because both iw and ig are checked before calling check_interleave_cap(). That means check_interleave_cap() will always get a eiw and a eig correctly.
they are checked in cxl_port_setup_targets() in auto-assembly case, otherwise checked in interleave_ways_store() and interleave_guranularity_store().
I am not sure if we need this patch, let's see if other reviewers have more comments.
Ming
>
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> ---
> drivers/cxl/core/region.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c3f4dc244df7..edf8636f190a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1266,8 +1266,8 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
> struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> unsigned int interleave_mask;
> - u8 eiw;
> - u16 eig;
> + u8 eiw = 0;
> + u16 eig = 0;
> int high_pos, low_pos;
>
> if (!test_bit(iw, &cxlhdm->iw_cap_mask))
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl: core: Initialize eiw and eig to fix potential uninitialized use
2025-04-20 10:43 ` Li Ming
@ 2025-04-21 15:40 ` Gregory Price
0 siblings, 0 replies; 6+ messages in thread
From: Gregory Price @ 2025-04-21 15:40 UTC (permalink / raw)
To: Li Ming
Cc: Purva Yeshi, huang.ying.caritas, linux-cxl, linux-kernel, dave,
jonathan.cameron, dave.jiang, alison.schofield, vishal.l.verma,
ira.weiny, dan.j.williams
On Sun, Apr 20, 2025 at 06:43:37PM +0800, Li Ming wrote:
> On 4/20/2025 4:35 AM, Purva Yeshi wrote:
> > Fix Smatch-detected issue:
> >
> > drivers/cxl/core/region.c:1292 check_interleave_cap()
> > error: uninitialized symbol 'eiw'.
> > drivers/cxl/core/region.c:1297 check_interleave_cap()
> > error: uninitialized symbol 'eig'.
> > drivers/cxl/core/region.c:1299 check_interleave_cap()
> > error: uninitialized symbol 'eig'.
> >
> > Smatch reports possible uninitialized usage of these variables if
> > the helper functions ways_to_eiw() or granularity_to_eig() fail to
> > assign values under certain conditions.
> >
> > Initialize the eiw and eig variables to zero when they are declared
> > in check_interleave_cap() to prevent potential use of uninitialized
> > values.
>
> I also met them during checking cxl drivers code with code inspection tools. But they are not real issues, because both iw and ig are checked before calling check_interleave_cap(). That means check_interleave_cap() will always get a eiw and a eig correctly.
>
> they are checked in cxl_port_setup_targets() in auto-assembly case, otherwise checked in interleave_ways_store() and interleave_guranularity_store().
>
unlikely, but things can change and if we know about this now we should
just fix it to avoid someone tripping over it in the future
~Gregory
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl: core: Initialize eiw and eig to fix potential uninitialized use
2025-04-19 20:35 [PATCH] cxl: core: Initialize eiw and eig to fix potential uninitialized use Purva Yeshi
2025-04-20 10:43 ` Li Ming
@ 2025-04-21 15:41 ` Gregory Price
2025-04-21 17:39 ` Dan Williams
2 siblings, 0 replies; 6+ messages in thread
From: Gregory Price @ 2025-04-21 15:41 UTC (permalink / raw)
To: Purva Yeshi
Cc: dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams, ming.li,
huang.ying.caritas, linux-cxl, linux-kernel
On Sun, Apr 20, 2025 at 02:05:30AM +0530, Purva Yeshi wrote:
> Fix Smatch-detected issue:
>
> drivers/cxl/core/region.c:1292 check_interleave_cap()
> error: uninitialized symbol 'eiw'.
> drivers/cxl/core/region.c:1297 check_interleave_cap()
> error: uninitialized symbol 'eig'.
> drivers/cxl/core/region.c:1299 check_interleave_cap()
> error: uninitialized symbol 'eig'.
>
> Smatch reports possible uninitialized usage of these variables if
> the helper functions ways_to_eiw() or granularity_to_eig() fail to
> assign values under certain conditions.
>
> Initialize the eiw and eig variables to zero when they are declared
> in check_interleave_cap() to prevent potential use of uninitialized
> values.
>
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
> ---
> drivers/cxl/core/region.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c3f4dc244df7..edf8636f190a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1266,8 +1266,8 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
> struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> unsigned int interleave_mask;
> - u8 eiw;
> - u16 eig;
> + u8 eiw = 0;
> + u16 eig = 0;
> int high_pos, low_pos;
>
> if (!test_bit(iw, &cxlhdm->iw_cap_mask))
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl: core: Initialize eiw and eig to fix potential uninitialized use
2025-04-19 20:35 [PATCH] cxl: core: Initialize eiw and eig to fix potential uninitialized use Purva Yeshi
2025-04-20 10:43 ` Li Ming
2025-04-21 15:41 ` Gregory Price
@ 2025-04-21 17:39 ` Dan Williams
2025-05-13 18:52 ` Alison Schofield
2 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2025-04-21 17:39 UTC (permalink / raw)
To: Purva Yeshi, dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams
Cc: ming.li, huang.ying.caritas, linux-cxl, linux-kernel, Purva Yeshi
Purva Yeshi wrote:
> Fix Smatch-detected issue:
>
> drivers/cxl/core/region.c:1292 check_interleave_cap()
> error: uninitialized symbol 'eiw'.
> drivers/cxl/core/region.c:1297 check_interleave_cap()
> error: uninitialized symbol 'eig'.
> drivers/cxl/core/region.c:1299 check_interleave_cap()
> error: uninitialized symbol 'eig'.
>
> Smatch reports possible uninitialized usage of these variables if
> the helper functions ways_to_eiw() or granularity_to_eig() fail to
> assign values under certain conditions.
>
> Initialize the eiw and eig variables to zero when they are declared
> in check_interleave_cap() to prevent potential use of uninitialized
> values.
>
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> ---
> drivers/cxl/core/region.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c3f4dc244df7..edf8636f190a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1266,8 +1266,8 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
> struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> unsigned int interleave_mask;
> - u8 eiw;
> - u16 eig;
> + u8 eiw = 0;
> + u16 eig = 0;
I appreciate that this quiets the warning, but it is leaving a trip
hazard for future code refactoring. I would prefer to make this scenario
harder to repeat in the future with something like:
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index be8a7dc77719..5f20919207ae 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -107,6 +107,7 @@ static inline int eiw_to_ways(u8 eiw, unsigned int *ways)
static inline int granularity_to_eig(int granularity, u16 *eig)
{
+ *eig = 0;
if (granularity > SZ_16K || granularity < CXL_DECODER_MIN_GRANULARITY ||
!is_power_of_2(granularity))
return -EINVAL;
@@ -116,6 +117,7 @@ static inline int granularity_to_eig(int granularity, u16 *eig)
static inline int ways_to_eiw(unsigned int ways, u8 *eiw)
{
+ *eiw = 0;
if (ways > 16)
return -EINVAL;
if (is_power_of_2(ways)) {
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] cxl: core: Initialize eiw and eig to fix potential uninitialized use
2025-04-21 17:39 ` Dan Williams
@ 2025-05-13 18:52 ` Alison Schofield
0 siblings, 0 replies; 6+ messages in thread
From: Alison Schofield @ 2025-05-13 18:52 UTC (permalink / raw)
To: Dan Williams
Cc: Purva Yeshi, dave, jonathan.cameron, dave.jiang, vishal.l.verma,
ira.weiny, ming.li, huang.ying.caritas, linux-cxl, linux-kernel
On Mon, Apr 21, 2025 at 10:39:38AM -0700, Dan Williams wrote:
> Purva Yeshi wrote:
> > Fix Smatch-detected issue:
> >
> > drivers/cxl/core/region.c:1292 check_interleave_cap()
> > error: uninitialized symbol 'eiw'.
> > drivers/cxl/core/region.c:1297 check_interleave_cap()
> > error: uninitialized symbol 'eig'.
> > drivers/cxl/core/region.c:1299 check_interleave_cap()
> > error: uninitialized symbol 'eig'.
> >
> > Smatch reports possible uninitialized usage of these variables if
> > the helper functions ways_to_eiw() or granularity_to_eig() fail to
> > assign values under certain conditions.
> >
> > Initialize the eiw and eig variables to zero when they are declared
> > in check_interleave_cap() to prevent potential use of uninitialized
> > values.
> >
Hi Purva,
We'd like to see this patch revised and resubmitted. It is a
good cleanup!
Go ahead and send a v2 with Dan's suggestion as the solution.
There's a couple of options for the tags. Here I think it is
good to add this tag:
Suggested-by: Dan Williams <dan.j.williams@intel.com>
followed by your Signed-off-by tag.
> > Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> > ---
You'll add a changelog in v2 here, below the ---, something like:
Changes in v2:
- init eiw and eig in their helper functions (Dan)
Thanks,
Alison
> > drivers/cxl/core/region.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index c3f4dc244df7..edf8636f190a 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1266,8 +1266,8 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
> > struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> > unsigned int interleave_mask;
> > - u8 eiw;
> > - u16 eig;
> > + u8 eiw = 0;
> > + u16 eig = 0;
>
> I appreciate that this quiets the warning, but it is leaving a trip
> hazard for future code refactoring. I would prefer to make this scenario
> harder to repeat in the future with something like:
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index be8a7dc77719..5f20919207ae 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -107,6 +107,7 @@ static inline int eiw_to_ways(u8 eiw, unsigned int *ways)
>
> static inline int granularity_to_eig(int granularity, u16 *eig)
> {
> + *eig = 0;
> if (granularity > SZ_16K || granularity < CXL_DECODER_MIN_GRANULARITY ||
> !is_power_of_2(granularity))
> return -EINVAL;
> @@ -116,6 +117,7 @@ static inline int granularity_to_eig(int granularity, u16 *eig)
>
> static inline int ways_to_eiw(unsigned int ways, u8 *eiw)
> {
> + *eiw = 0;
> if (ways > 16)
> return -EINVAL;
> if (is_power_of_2(ways)) {
^ permalink raw reply [flat|nested] 6+ messages in thread