Re: [bug report] drm/xen-front: Add support for Xen PV display frontend

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Tue, Apr 21, 2020 at 08:59:09PM +0200, Julia Lawall wrote:
> 
> 
> On Tue, 21 Apr 2020, Dan Carpenter wrote:
> 
> > On Tue, Apr 21, 2020 at 05:29:02PM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Tue, 21 Apr 2020, Dan Carpenter wrote:
> > >
> > > > Hi Kernel Janitors,
> > > >
> > > > Here is another idea that someone could work on, fixing the
> > > > IS_ERR_OR_NULL() checks in the xen driver.
> > > >
> > > > The patch c575b7eeb89f: "drm/xen-front: Add support for Xen PV
> > > > display frontend" from Apr 3, 2018, leads to the following static
> > > > checker warning:
> > > >
> > > >         drivers/gpu/drm/xen/xen_drm_front_gem.c:140 
> > > > xen_drm_front_gem_create()
> > > >         warn: passing zero to 'ERR_CAST'
> > > >
> > > > drivers/gpu/drm/xen/xen_drm_front_gem.c
> > > >    133  struct drm_gem_object *xen_drm_front_gem_create(struct 
> > > > drm_device *dev,
> > > >    134                                                  size_t size)
> > > >    135  {
> > > >    136          struct xen_gem_object *xen_obj;
> > > >    137
> > > >    138          xen_obj = gem_create(dev, size);
> > > >    139          if (IS_ERR_OR_NULL(xen_obj))
> > > >    140                  return ERR_CAST(xen_obj);
> > >
> > > Are the other occurrences of this also a possible problem?  There are a
> > > few others outside of xen.
> >
> > We sometimes check a parameter for IS_ERR_OR_NULL().
> >
> > void free_function(struct something *p)
> > {
> >     if (IS_ERR_OR_NULL(p))
> >             return;
> > }
> >
> > That's fine, absolutely harmless and not a bug.  But if we are checking
> > a return value like this then probably most of the time it's invalid
> > code.  Normally it's again like this code where we're dealing with an
> > impossible thing because the return is never NULL.  The common bugs are
> > that it returns NULL to a caller which only expects error pointers or it
> > returns success instead of failure.  But sometimes returning success can
> > be valid:
> >
> >     obj = get_feature(dev);
> >     if (IS_ERR_OR_NULL(obj))
> >             return PTR_ERR(obj);
> >
> > It deliberately returns success because the rest of the function is
> > useless when we don't have the feature.
> 
> The other cases are also with ERR_CAST:

I bet these are less likely to be correct because probably the optional
feature doesn't translate to a different optional feature.

> 
> drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c in create_udp_flow

This can never be NULL, but look at this code:

drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c
   427                          if (trans_spec) {
   428                                  qp_flow = create_and_add_flow(qp_grp,
   429                                                                  
trans_spec);
   430                                  if (IS_ERR_OR_NULL(qp_flow)) {
   431                                          status = qp_flow ? 
PTR_ERR(qp_flow) : -EFAULT;
   432                                          break;
   433                                  }
   434                          } else {

If the create_and_add_flow() returns NULL, that's a bug because it's not
an optional feature which can be disabled in the config etc.  But this
code has been future proofed in case future users decide to write buggy
code the NULL gets changed to an error code.

Is -EFAULT the correct way to handle bugs that future programmers are
going to introduce?  You have to very psychic to know the answer to
that.

> fs/overlayfs/namei.c in ovl_index_upper

This code is correct.  There are actually a bunch of functions in VFS
which only return NULL and error pointers, never valid pointers.  VFS
is weird like that.

> sound/soc/qcom/qdsp6/q6adm.c in q6adm_open

Never NULL.  It should be IS_ERR().

> drivers/clk/clk.c in clk_hw_create_clk

This is checking a parameter so it's fine.

regards,
dan carpenter