Re: [net v1] devlink: fix xa_alloc_cyclic error handling

On Fri, Feb 14, 2025 at 02:44:49PM +0100, Andrew Lunn wrote:

On Fri, Feb 14, 2025 at 02:24:53PM +0100, Michal Swiatkowski wrote:

quoted

Pierre Riteau [off-list ref] found suspicious handling an error
from xa_alloc_cyclic() in scheduler code [1]. The same is done in
devlink_rel_alloc().

If the same bug exists twice it might exist more times. Did you find
this instance by searching the whole tree? Or just networking?

This is also something which would be good to have the static
analysers check for. I wounder if smatch can check this?

That's a great idea, thanks!  I'll try a couple experiments and see what
works tomorrow.  I've add these lines to check_zero_to_err_ptr.c

   183          max = rl_max(estate_rl(sm->state));
   184          if (max.value > 0 && !sval_is_a_max(max))
   185                  sm_warning("passing non-max range '%s' to '%s'", sm->state->name, fn);
   186  

I'm hoping this one works.  It complains about any positive returns
except for when the return is "some non-zero value".

   194                  if (estate_get_single_value(tmp->state, &sval) &&
   195                      (sval.value < -4096 || sval.value > 0)) {
   196                          sm_warning("passing invalid error code %lld to '%s'", sval.value, fn);
   197                          return;
   198                  }

This one might miss some bugs but it should catch most stuff and have few
false positives.  Both of them work on this example.

net/devlink/core.c:122 devlink_rel_alloc() warn: passing non-max range '(-4095)-(-1),1' to 'ERR_PTR'
net/devlink/core.c:122 devlink_rel_alloc() warn: passing invalid error code 1 to 'ERR_PTR'

regards,
dan carpenter