[bug report] firmware: arm_scmi: Add notification callbacks-registration

[bug report] firmware: arm_scmi: Add notification callbacks-registration

[Dan Carpenter]

Hello Cristian Marussi,

The patch 5b352c537930: "firmware: arm_scmi: Add notification
callbacks-registration" from Jun 19, 2020, leads to the following
static checker warning:

	drivers/firmware/arm_scmi/notify.c:1267 scmi_register_notifier()
	warn: passing zero to 'PTR_ERR'

drivers/firmware/arm_scmi/notify.c
  1248  static int scmi_register_notifier(const struct scmi_handle *handle,
  1249                                    u8 proto_id, u8 evt_id, u32 *src_id,
  1250                                    struct notifier_block *nb)
  1251  {
  1252          int ret = 0;
  1253          u32 evt_key;
  1254          struct scmi_event_handler *hndl;
  1255          struct scmi_notify_instance *ni;
  1256  
  1257          /* Ensure notify_priv is updated */
  1258          smp_rmb();
  1259          if (unlikely(!handle->notify_priv))
  1260                  return -ENODEV;
  1261          ni = handle->notify_priv;
  1262  
  1263          evt_key = MAKE_HASH_KEY(proto_id, evt_id,
  1264                                  src_id ? *src_id : SRC_ID_MASK);
  1265          hndl = scmi_get_or_create_handler(ni, evt_key);
  1266          if (IS_ERR_OR_NULL(hndl))
                    ^^^^^^^^^^^^^^^^^^^^
There are a lot of wrong uses of IS_ERR_OR_NULL() in this driver.

When a function returns both NULL and error pointers, then NULL is a
special kind of success.  For example, we could have code which does
"p = get_feature();" and maybe there is an allocation failure, then
get_feature() should return an error pointer and we return that to the
user.

But if the get_feature() is optional and it has been deliberately
disabled by the user with CONFIG_FOO=n then that is *not* an error.  But
we are still not able to return a valid pointer to the feature so it
returns NULL.  The driver should continue to operate without the
optional feature.

  1267                  return PTR_ERR(hndl);

In this situation the scmi_get_or_create_handler() never returns error
pointers.  It's not an optional feature.  It returns NULL on failure.
So that means we return PTR_ERR(NULL) which is zero which means success.
The code should instead a negative error code instead:

	if (!hndl)
		return -EINVAL;

All the uses of IS_ERR_OR_NULL() that I saw were wrong but this is the
only one that caused a user visible bug.

  1268  
  1269          blocking_notifier_chain_register(&hndl->chain, nb);
  1270  
  1271          /* Enable events for not pending handlers */
  1272          if (likely(!IS_HNDL_PENDING(hndl))) {
                    ^^^^^^
The likely/unlikely() annotations should only be used where it afects
benchmarking.  They should all be removed from this driver.

  1273                  if (!scmi_event_handler_enable_events(hndl)) {

I really don't like that half these functions follow normal kernel error
style and half return true/false on failure.  Normally we would want
boolean functions to be clear from the name like access_ok() which
clearly returns true/false. But I didn't see that it causes any bugs
yet.

One thing I did notice is that scmi_sensors_init() return non-zero
positive values on success because it's returning the result from
idr_alloc().  This should trigger a warning message, I believe.

  1274                          scmi_put_handler(ni, hndl);
  1275                          ret = -EINVAL;
  1276                  }
  1277          }
  1278  
  1279          return ret;
  1280  }

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [bug report] firmware: arm_scmi: Add notification callbacks-registration

[Cristian Marussi]

On Tue, Jun 30, 2020 at 08:57:18PM +0300, Dan Carpenter wrote:
> Hello Cristian Marussi,

Hi Dan Carpenter

thanks for the feedback first of all.

> 
> The patch 5b352c537930: "firmware: arm_scmi: Add notification
> callbacks-registration" from Jun 19, 2020, leads to the following
> static checker warning:
> 
> 	drivers/firmware/arm_scmi/notify.c:1267 scmi_register_notifier()
> 	warn: passing zero to 'PTR_ERR'
> 
> drivers/firmware/arm_scmi/notify.c
>   1248  static int scmi_register_notifier(const struct scmi_handle *handle,
>   1249                                    u8 proto_id, u8 evt_id, u32 *src_id,
>   1250                                    struct notifier_block *nb)
>   1251  {
>   1252          int ret = 0;
>   1253          u32 evt_key;
>   1254          struct scmi_event_handler *hndl;
>   1255          struct scmi_notify_instance *ni;
>   1256  
>   1257          /* Ensure notify_priv is updated */
>   1258          smp_rmb();
>   1259          if (unlikely(!handle->notify_priv))
>   1260                  return -ENODEV;
>   1261          ni = handle->notify_priv;
>   1262  
>   1263          evt_key = MAKE_HASH_KEY(proto_id, evt_id,
>   1264                                  src_id ? *src_id : SRC_ID_MASK);
>   1265          hndl = scmi_get_or_create_handler(ni, evt_key);
>   1266          if (IS_ERR_OR_NULL(hndl))
>                     ^^^^^^^^^^^^^^^^^^^^
> There are a lot of wrong uses of IS_ERR_OR_NULL() in this driver.
> 
> When a function returns both NULL and error pointers, then NULL is a
> special kind of success.  For example, we could have code which does
> "p = get_feature();" and maybe there is an allocation failure, then
> get_feature() should return an error pointer and we return that to the
> user.
> 
> But if the get_feature() is optional and it has been deliberately
> disabled by the user with CONFIG_FOO=n then that is *not* an error.  But
> we are still not able to return a valid pointer to the feature so it
> returns NULL.  The driver should continue to operate without the
> optional feature.
> 
>   1267                  return PTR_ERR(hndl);
> 
> In this situation the scmi_get_or_create_handler() never returns error
> pointers.  It's not an optional feature.  It returns NULL on failure.
> So that means we return PTR_ERR(NULL) which is zero which means success.
> The code should instead a negative error code instead:
> 
> 	if (!hndl)
> 		return -EINVAL;
> 
> All the uses of IS_ERR_OR_NULL() that I saw were wrong but this is the
> only one that caused a user visible bug.

I did not know about this intended usage, I'm removing all these unneded
IS_ERR_OR_NULL() usages with plain NULL checks.

> 
>   1268  
>   1269          blocking_notifier_chain_register(&hndl->chain, nb);
>   1270  
>   1271          /* Enable events for not pending handlers */
>   1272          if (likely(!IS_HNDL_PENDING(hndl))) {
>                     ^^^^^^
> The likely/unlikely() annotations should only be used where it afects
> benchmarking.  They should all be removed from this driver.

I'm removing all the likely/unlikely from this series.

> 
>   1273                  if (!scmi_event_handler_enable_events(hndl)) {
> 
> I really don't like that half these functions follow normal kernel error
> style and half return true/false on failure.  Normally we would want
> boolean functions to be clear from the name like access_ok() which
> clearly returns true/false. But I didn't see that it causes any bugs
> yet.

Ok I'll convert all the functions to classical 0-Success style dumping
the booleans. I cannot see in fact any suitable func_ok() style function.

> 
> One thing I did notice is that scmi_sensors_init() return non-zero
> positive values on success because it's returning the result from
> idr_alloc().  This should trigger a warning message, I believe.
> 
>   1274                          scmi_put_handler(ni, hndl);
>   1275                          ret = -EINVAL;
>   1276                  }
>   1277          }
>   1278  
>   1279          return ret;
>   1280  }
> 

The scmi_sensors_init() retval is not part of this series so I'll make
a note of it but do not change it right now, also because I'm also reviewing
more generally the whole SCMI initialization core, aiming at full modularization
of the protocols, so that this part is already changed/gone in another
unpublished series.

Thanks

Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [bug report] firmware: arm_scmi: Add notification callbacks-registration

[Dan Carpenter]

On Wed, Jul 01, 2020 at 01:29:24PM +0100, Cristian Marussi wrote:
> >   1273                  if (!scmi_event_handler_enable_events(hndl)) {
> > 
> > I really don't like that half these functions follow normal kernel error
> > style and half return true/false on failure.  Normally we would want
> > boolean functions to be clear from the name like access_ok() which
> > clearly returns true/false. But I didn't see that it causes any bugs
> > yet.
> 
> Ok I'll convert all the functions to classical 0-Success style dumping
> the booleans. I cannot see in fact any suitable func_ok() style function.

You are a hero.  Thanks!

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel