cxl: fix return value in cxlctl_validate_set_features()

[PATCH next] cxl: fix return value in cxlctl_validate_set_features()

Posted by Dan Carpenter 6 months, 3 weeks ago

The cxlctl_validate_set_features() function is type bool.  It's supposed
to return true for valid requests and false for invalid.  However, this
error path returns ERR_PTR(-EINVAL) which is true when it was intended to
return false.

Fixes: f76e0bbc8bc3 ("cxl: Update prototype of function get_support_feature_info()")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/cxl/core/features.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
index 6f2eae1eb126..7c750599ea69 100644
--- a/drivers/cxl/core/features.c
+++ b/drivers/cxl/core/features.c
@@ -544,7 +544,7 @@ static bool cxlctl_validate_set_features(struct cxl_features_state *cxlfs,
 	u32 flags;
 
 	if (rpc_in->op_size < sizeof(uuid_t))
-		return ERR_PTR(-EINVAL);
+		return false;
 
 	feat = cxl_feature_info(cxlfs, &rpc_in->set_feat_in.uuid);
 	if (IS_ERR(feat))
-- 
2.47.2

Re: [PATCH next] cxl: fix return value in cxlctl_validate_set_features()

Posted by Dave Jiang 6 months, 1 week ago


On 5/28/25 1:11 AM, Dan Carpenter wrote:
> The cxlctl_validate_set_features() function is type bool.  It's supposed
> to return true for valid requests and false for invalid.  However, this
> error path returns ERR_PTR(-EINVAL) which is true when it was intended to
> return false.
> 
> Fixes: f76e0bbc8bc3 ("cxl: Update prototype of function get_support_feature_info()")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Applied to cxl/fixes

> ---
>  drivers/cxl/core/features.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 6f2eae1eb126..7c750599ea69 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -544,7 +544,7 @@ static bool cxlctl_validate_set_features(struct cxl_features_state *cxlfs,
>  	u32 flags;
>  
>  	if (rpc_in->op_size < sizeof(uuid_t))
> -		return ERR_PTR(-EINVAL);
> +		return false;
>  
>  	feat = cxl_feature_info(cxlfs, &rpc_in->set_feat_in.uuid);
>  	if (IS_ERR(feat))

Re: [PATCH next] cxl: fix return value in cxlctl_validate_set_features()

Posted by Alison Schofield 6 months, 3 weeks ago

On Wed, May 28, 2025 at 11:11:41AM +0300, Dan Carpenter wrote:
> The cxlctl_validate_set_features() function is type bool.  It's supposed
> to return true for valid requests and false for invalid.  However, this
> error path returns ERR_PTR(-EINVAL) which is true when it was intended to
> return false.

Shiju - Can you trace this one through and add the impact statement?
Wondering if this is going to fail gracefully, or badly, further 
down this path?

> 
> Fixes: f76e0bbc8bc3 ("cxl: Update prototype of function get_support_feature_info()")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/cxl/core/features.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 6f2eae1eb126..7c750599ea69 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -544,7 +544,7 @@ static bool cxlctl_validate_set_features(struct cxl_features_state *cxlfs,
>  	u32 flags;
>  
>  	if (rpc_in->op_size < sizeof(uuid_t))
> -		return ERR_PTR(-EINVAL);
> +		return false;
>  
>  	feat = cxl_feature_info(cxlfs, &rpc_in->set_feat_in.uuid);
>  	if (IS_ERR(feat))
> -- 
> 2.47.2
>

RE: [PATCH next] cxl: fix return value in cxlctl_validate_set_features()

Posted by Shiju Jose 6 months, 3 weeks ago

>-----Original Message-----
>From: Alison Schofield <alison.schofield@intel.com>
>Sent: 28 May 2025 16:23
>To: Shiju Jose <shiju.jose@huawei.com>; Dan Carpenter
><dan.carpenter@linaro.org>
>Cc: Davidlohr Bueso <dave@stgolabs.net>; Jonathan Cameron
><jonathan.cameron@huawei.com>; Dave Jiang <dave.jiang@intel.com>; Vishal
>Verma <vishal.l.verma@intel.com>; Ira Weiny <ira.weiny@intel.com>; Dan
>Williams <dan.j.williams@intel.com>; Li Ming <ming.li@zohomail.com>; Fan Ni
><fan.ni@samsung.com>; linux-cxl@vger.kernel.org; linux-
>kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
>Subject: Re: [PATCH next] cxl: fix return value in cxlctl_validate_set_features()
>
>On Wed, May 28, 2025 at 11:11:41AM +0300, Dan Carpenter wrote:
>> The cxlctl_validate_set_features() function is type bool.  It's
>> supposed to return true for valid requests and false for invalid.
>> However, this error path returns ERR_PTR(-EINVAL) which is true when
>> it was intended to return false.
>
>Shiju - Can you trace this one through and add the impact statement?
>Wondering if this is going to fail gracefully, or badly, further down this path?

Hi Alison,

This is introduced when following fwctl specific code
move out of common  function (use both in fwctl and edac path)
get_support_feature_info() to fwctl specific function
cxlctl_validae_set_feature().
"if (rpc_in->op_size < sizeof(uuid_t))
      return ERR_PTR(-EINVAL);"

This may have an impact on fwctl side if the above check pass.

Thanks,
Shiju

>
>>
>> Fixes: f76e0bbc8bc3 ("cxl: Update prototype of function
>> get_support_feature_info()")
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> ---
>>  drivers/cxl/core/features.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
>> index 6f2eae1eb126..7c750599ea69 100644
>> --- a/drivers/cxl/core/features.c
>> +++ b/drivers/cxl/core/features.c
>> @@ -544,7 +544,7 @@ static bool cxlctl_validate_set_features(struct
>cxl_features_state *cxlfs,
>>  	u32 flags;
>>
>>  	if (rpc_in->op_size < sizeof(uuid_t))
>> -		return ERR_PTR(-EINVAL);
>> +		return false;
>>
>>  	feat = cxl_feature_info(cxlfs, &rpc_in->set_feat_in.uuid);
>>  	if (IS_ERR(feat))
>> --
>> 2.47.2
>>

RE: [PATCH next] cxl: fix return value in cxlctl_validate_set_features()

Posted by Ira Weiny 6 months, 1 week ago

Shiju Jose wrote:
> >-----Original Message-----
> >From: Alison Schofield <alison.schofield@intel.com>
> >Sent: 28 May 2025 16:23
> >To: Shiju Jose <shiju.jose@huawei.com>; Dan Carpenter
> ><dan.carpenter@linaro.org>
> >Cc: Davidlohr Bueso <dave@stgolabs.net>; Jonathan Cameron
> ><jonathan.cameron@huawei.com>; Dave Jiang <dave.jiang@intel.com>; Vishal
> >Verma <vishal.l.verma@intel.com>; Ira Weiny <ira.weiny@intel.com>; Dan
> >Williams <dan.j.williams@intel.com>; Li Ming <ming.li@zohomail.com>; Fan Ni
> ><fan.ni@samsung.com>; linux-cxl@vger.kernel.org; linux-
> >kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
> >Subject: Re: [PATCH next] cxl: fix return value in cxlctl_validate_set_features()
> >
> >On Wed, May 28, 2025 at 11:11:41AM +0300, Dan Carpenter wrote:
> >> The cxlctl_validate_set_features() function is type bool.  It's
> >> supposed to return true for valid requests and false for invalid.
> >> However, this error path returns ERR_PTR(-EINVAL) which is true when
> >> it was intended to return false.
> >
> >Shiju - Can you trace this one through and add the impact statement?
> >Wondering if this is going to fail gracefully, or badly, further down this path?
> 
> Hi Alison,
> 
> This is introduced when following fwctl specific code
> move out of common  function (use both in fwctl and edac path)
> get_support_feature_info() to fwctl specific function
> cxlctl_validae_set_feature().
> "if (rpc_in->op_size < sizeof(uuid_t))
>       return ERR_PTR(-EINVAL);"
> 
> This may have an impact on fwctl side if the above check pass.
> 

I got a bit sidetracked by this conversation.

It seems the TLDR is:

This fix is not required.  But should be fixed for long term correctness.

With that interpretation.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Re: [PATCH next] cxl: fix return value in cxlctl_validate_set_features()

Posted by Dan Carpenter 6 months, 3 weeks ago

On Wed, May 28, 2025 at 08:22:35AM -0700, Alison Schofield wrote:
> On Wed, May 28, 2025 at 11:11:41AM +0300, Dan Carpenter wrote:
> > The cxlctl_validate_set_features() function is type bool.  It's supposed
> > to return true for valid requests and false for invalid.  However, this
> > error path returns ERR_PTR(-EINVAL) which is true when it was intended to
> > return false.
> 
> Shiju - Can you trace this one through and add the impact statement?
> Wondering if this is going to fail gracefully, or badly, further 
> down this path?
> 

Sorry, I would normally analyse this a bit more myself, but it's only in
linux-next so I assumed no one was using it yet.  It ends up being fine.

cxlctl_set_feature() has a check for:

	if (rpc_in->op_size <= sizeof(feat_in->hdr))

at the start and sizeof(feat_in->hdr) is larger than sizeof(uuid_t).

regards,
dan carpenter