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

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

[Dan Carpenter]

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);

This warning is old and it's actually the result of a bug I had in my
devel version of Smatch yesterday.  xen_obj can't really be NULL, but
if it were then the caller would return success which would probably
create some issues.

When a function returns both error pointers and NULL then NULL is a
special case of success.  Like say you have:  "p = start_feature();".
If there is an allocation failure, then the code should return -ENOMEM
and the whole thing should fail.  But if the feature is optional and
the user has disabled it in the config then we return NULL and the
caller has to be able to accept that.  There are a lot of these
IS_ERR_OR_NULL() checks in the xen driver...

The one here is clearly buggy because returning NULL would lead to a
run time failure.  All these IS_ERR_OR_NULL() should be checked and
probably changed to just IS_ERR().

This sort of change is probably change is probably easiest if you build
the Smatch DB and you can check if Smatch thinks the functions return
NULL.

~/smatch/smatch_data/db/smdb.py return_states gem_create | grep INTERNAL
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 58 |  (-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 62 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 66 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 68 | 0,(-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |

Smatch thinks that gem_create() sometimes returns NULL or error pointers
but that's because of a bug in the unreleased version of Smatch and
because gem_create() uses IS_ERR_OR_NULL().

   141  
   142          return &xen_obj->base;
   143  }

regards,
dan carpenter

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

[Dan Carpenter]

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);

This warning is old and it's actually the result of a bug I had in my
devel version of Smatch yesterday.  xen_obj can't really be NULL, but
if it were then the caller would return success which would probably
create some issues.

When a function returns both error pointers and NULL then NULL is a
special case of success.  Like say you have:  "p = start_feature();".
If there is an allocation failure, then the code should return -ENOMEM
and the whole thing should fail.  But if the feature is optional and
the user has disabled it in the config then we return NULL and the
caller has to be able to accept that.  There are a lot of these
IS_ERR_OR_NULL() checks in the xen driver...

The one here is clearly buggy because returning NULL would lead to a
run time failure.  All these IS_ERR_OR_NULL() should be checked and
probably changed to just IS_ERR().

This sort of change is probably change is probably easiest if you build
the Smatch DB and you can check if Smatch thinks the functions return
NULL.

~/smatch/smatch_data/db/smdb.py return_states gem_create | grep INTERNAL
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 58 |  (-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 62 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 66 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 68 | 0,(-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |

Smatch thinks that gem_create() sometimes returns NULL or error pointers
but that's because of a bug in the unreleased version of Smatch and
because gem_create() uses IS_ERR_OR_NULL().

   141  
   142          return &xen_obj->base;
   143  }

regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Dan Carpenter]

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);

This warning is old and it's actually the result of a bug I had in my
devel version of Smatch yesterday.  xen_obj can't really be NULL, but
if it were then the caller would return success which would probably
create some issues.

When a function returns both error pointers and NULL then NULL is a
special case of success.  Like say you have:  "p = start_feature();".
If there is an allocation failure, then the code should return -ENOMEM
and the whole thing should fail.  But if the feature is optional and
the user has disabled it in the config then we return NULL and the
caller has to be able to accept that.  There are a lot of these
IS_ERR_OR_NULL() checks in the xen driver...

The one here is clearly buggy because returning NULL would lead to a
run time failure.  All these IS_ERR_OR_NULL() should be checked and
probably changed to just IS_ERR().

This sort of change is probably change is probably easiest if you build
the Smatch DB and you can check if Smatch thinks the functions return
NULL.

~/smatch/smatch_data/db/smdb.py return_states gem_create | grep INTERNAL
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 58 |  (-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 62 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 66 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 68 | 0,(-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |

Smatch thinks that gem_create() sometimes returns NULL or error pointers
but that's because of a bug in the unreleased version of Smatch and
because gem_create() uses IS_ERR_OR_NULL().

   141  
   142          return &xen_obj->base;
   143  }

regards,
dan carpenter

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

[Oleksandr Andrushchenko]

T24gNC8yMS8yMCAxMzo0NSwgRGFuIENhcnBlbnRlciB3cm90ZToNCj4gSGkgS2VybmVsIEphbml0
b3JzLA0KSGkNCj4NCj4gSGVyZSBpcyBhbm90aGVyIGlkZWEgdGhhdCBzb21lb25lIGNvdWxkIHdv
cmsgb24sIGZpeGluZyB0aGUNCj4gSVNfRVJSX09SX05VTEwoKSBjaGVja3MgaW4gdGhlIHhlbiBk
cml2ZXIuDQo+DQo+IFRoZSBwYXRjaCBjNTc1YjdlZWI4OWY6ICJkcm0veGVuLWZyb250OiBBZGQg
c3VwcG9ydCBmb3IgWGVuIFBWDQo+IGRpc3BsYXkgZnJvbnRlbmQiIGZyb20gQXByIDMsIDIwMTgs
IGxlYWRzIHRvIHRoZSBmb2xsb3dpbmcgc3RhdGljDQo+IGNoZWNrZXIgd2FybmluZzoNCj4NCj4g
CWRyaXZlcnMvZ3B1L2RybS94ZW4veGVuX2RybV9mcm9udF9nZW0uYzoxNDAgeGVuX2RybV9mcm9u
dF9nZW1fY3JlYXRlKCkNCj4gCXdhcm46IHBhc3NpbmcgemVybyB0byAnRVJSX0NBU1QnDQo+DQo+
IGRyaXZlcnMvZ3B1L2RybS94ZW4veGVuX2RybV9mcm9udF9nZW0uYw0KPiAgICAgMTMzICBzdHJ1
Y3QgZHJtX2dlbV9vYmplY3QgKnhlbl9kcm1fZnJvbnRfZ2VtX2NyZWF0ZShzdHJ1Y3QgZHJtX2Rl
dmljZSAqZGV2LA0KPiAgICAgMTM0ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICBzaXplX3Qgc2l6ZSkNCj4gICAgIDEzNSAgew0KPiAgICAgMTM2ICAgICAg
ICAgIHN0cnVjdCB4ZW5fZ2VtX29iamVjdCAqeGVuX29iajsNCj4gICAgIDEzNw0KPiAgICAgMTM4
ICAgICAgICAgIHhlbl9vYmogPSBnZW1fY3JlYXRlKGRldiwgc2l6ZSk7DQo+ICAgICAxMzkgICAg
ICAgICAgaWYgKElTX0VSUl9PUl9OVUxMKHhlbl9vYmopKQ0KPiAgICAgMTQwICAgICAgICAgICAg
ICAgICAgcmV0dXJuIEVSUl9DQVNUKHhlbl9vYmopOw0KPg0KPiBUaGlzIHdhcm5pbmcgaXMgb2xk
IGFuZCBpdCdzIGFjdHVhbGx5IHRoZSByZXN1bHQgb2YgYSBidWcgSSBoYWQgaW4gbXkNCj4gZGV2
ZWwgdmVyc2lvbiBvZiBTbWF0Y2ggeWVzdGVyZGF5LiAgeGVuX29iaiBjYW4ndCByZWFsbHkgYmUg
TlVMTCwgYnV0DQo+IGlmIGl0IHdlcmUgdGhlbiB0aGUgY2FsbGVyIHdvdWxkIHJldHVybiBzdWNj
ZXNzIHdoaWNoIHdvdWxkIHByb2JhYmx5DQo+IGNyZWF0ZSBzb21lIGlzc3Vlcy4NCj4NCj4gV2hl
biBhIGZ1bmN0aW9uIHJldHVybnMgYm90aCBlcnJvciBwb2ludGVycyBhbmQgTlVMTCB0aGVuIE5V
TEwgaXMgYQ0KPiBzcGVjaWFsIGNhc2Ugb2Ygc3VjY2Vzcy4gIExpa2Ugc2F5IHlvdSBoYXZlOiAg
InAgPSBzdGFydF9mZWF0dXJlKCk7Ii4NCj4gSWYgdGhlcmUgaXMgYW4gYWxsb2NhdGlvbiBmYWls
dXJlLCB0aGVuIHRoZSBjb2RlIHNob3VsZCByZXR1cm4gLUVOT01FTQ0KPiBhbmQgdGhlIHdob2xl
IHRoaW5nIHNob3VsZCBmYWlsLiAgQnV0IGlmIHRoZSBmZWF0dXJlIGlzIG9wdGlvbmFsIGFuZA0K
PiB0aGUgdXNlciBoYXMgZGlzYWJsZWQgaXQgaW4gdGhlIGNvbmZpZyB0aGVuIHdlIHJldHVybiBO
VUxMIGFuZCB0aGUNCj4gY2FsbGVyIGhhcyB0byBiZSBhYmxlIHRvIGFjY2VwdCB0aGF0LiAgVGhl
cmUgYXJlIGEgbG90IG9mIHRoZXNlDQo+IElTX0VSUl9PUl9OVUxMKCkgY2hlY2tzIGluIHRoZSB4
ZW4gZHJpdmVyLi4uDQo+DQo+IFRoZSBvbmUgaGVyZSBpcyBjbGVhcmx5IGJ1Z2d5IGJlY2F1c2Ug
cmV0dXJuaW5nIE5VTEwgd291bGQgbGVhZCB0byBhDQo+IHJ1biB0aW1lIGZhaWx1cmUuICBBbGwg
dGhlc2UgSVNfRVJSX09SX05VTEwoKSBzaG91bGQgYmUgY2hlY2tlZCBhbmQNCj4gcHJvYmFibHkg
Y2hhbmdlZCB0byBqdXN0IElTX0VSUigpLg0KPg0KPiBUaGlzIHNvcnQgb2YgY2hhbmdlIGlzIHBy
b2JhYmx5IGNoYW5nZSBpcyBwcm9iYWJseSBlYXNpZXN0IGlmIHlvdSBidWlsZA0KPiB0aGUgU21h
dGNoIERCIGFuZCB5b3UgY2FuIGNoZWNrIGlmIFNtYXRjaCB0aGlua3MgdGhlIGZ1bmN0aW9ucyBy
ZXR1cm4NCj4gTlVMTC4NCj4NCj4gfi9zbWF0Y2gvc21hdGNoX2RhdGEvZGIvc21kYi5weSByZXR1
cm5fc3RhdGVzIGdlbV9jcmVhdGUgfCBncmVwIElOVEVSTkFMDQo+IGRyaXZlcnMvZ3B1L2RybS94
ZW4veGVuX2RybV9mcm9udF9nZW0uYyB8IGdlbV9jcmVhdGUgfCA1OCB8ICAoLTQwOTUpLSgtMSkg
fCAgICAgIElOVEVSTkFMIHwgIC0xIHwgICAgICAgICAgICAgICAgICAgICAgfCBzdHJ1Y3QgeGVu
X2dlbV9vYmplY3QqKCopKHN0cnVjdCBkcm1fZGV2aWNlKiwgdWxvbmcpIHwNCj4gZHJpdmVycy9n
cHUvZHJtL3hlbi94ZW5fZHJtX2Zyb250X2dlbS5jIHwgZ2VtX2NyZWF0ZSB8IDYyIHwgNDA2NTAz
NTg5Nzg0OTMwMzA0MCB8ICAgICAgSU5URVJOQUwgfCAgLTEgfCAgICAgICAgICAgICAgICAgICAg
ICB8IHN0cnVjdCB4ZW5fZ2VtX29iamVjdCooKikoc3RydWN0IGRybV9kZXZpY2UqLCB1bG9uZykg
fA0KPiBkcml2ZXJzL2dwdS9kcm0veGVuL3hlbl9kcm1fZnJvbnRfZ2VtLmMgfCBnZW1fY3JlYXRl
IHwgNjYgfCA0MDY1MDM1ODk3ODQ5MzAzMDQwIHwgICAgICBJTlRFUk5BTCB8ICAtMSB8ICAgICAg
ICAgICAgICAgICAgICAgIHwgc3RydWN0IHhlbl9nZW1fb2JqZWN0KigqKShzdHJ1Y3QgZHJtX2Rl
dmljZSosIHVsb25nKSB8DQo+IGRyaXZlcnMvZ3B1L2RybS94ZW4veGVuX2RybV9mcm9udF9nZW0u
YyB8IGdlbV9jcmVhdGUgfCA2OCB8IDAsKC00MDk1KS0oLTEpIHwgICAgICBJTlRFUk5BTCB8ICAt
MSB8ICAgICAgICAgICAgICAgICAgICAgIHwgc3RydWN0IHhlbl9nZW1fb2JqZWN0KigqKShzdHJ1
Y3QgZHJtX2RldmljZSosIHVsb25nKSB8DQo+DQo+IFNtYXRjaCB0aGlua3MgdGhhdCBnZW1fY3Jl
YXRlKCkgc29tZXRpbWVzIHJldHVybnMgTlVMTCBvciBlcnJvciBwb2ludGVycw0KPiBidXQgdGhh
dCdzIGJlY2F1c2Ugb2YgYSBidWcgaW4gdGhlIHVucmVsZWFzZWQgdmVyc2lvbiBvZiBTbWF0Y2gg
YW5kDQo+IGJlY2F1c2UgZ2VtX2NyZWF0ZSgpIHVzZXMgSVNfRVJSX09SX05VTEwoKS4NCj4NCj4g
ICAgIDE0MQ0KPiAgICAgMTQyICAgICAgICAgIHJldHVybiAmeGVuX29iai0+YmFzZTsNCj4gICAg
IDE0MyAgfQ0KPg0KPiByZWdhcmRzLA0KPiBkYW4gY2FycGVudGVyDQoNClRoYW5rIHlvdSBmb3Ig
dGhlIHJlcG9ydCwgSSB3aWxsIHRyeSB0byBmaW5kIHNvbWUgdGltZSB0byBsb29rIGludG8gdGhp
cyANCmFuZCBjb21lIHVwIHdpdGggZml4ZXMuDQoNCkNvdWxkIHlvdSBwbGVhc2UgKHByb2JhYmx5
IG9mZi1saXN0KSBpbnN0cnVjdCBtZSBvciBnaXZlIGFueSBwb2ludGVycyB0byANCmhvdyB0byBy
ZXByb2R1Y2UNCg0KdGhlIHJlc3VsdHMgb2YgdGhlIGFuYWx5emVyIHNob3duIGFib3ZlPw0KDQpU
aGFuayB5b3UsDQoNCk9sZWtzYW5kcg0K

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

[Oleksandr Andrushchenko]

On 4/21/20 13:45, Dan Carpenter wrote:
> Hi Kernel Janitors,
Hi
>
> 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);
>
> This warning is old and it's actually the result of a bug I had in my
> devel version of Smatch yesterday.  xen_obj can't really be NULL, but
> if it were then the caller would return success which would probably
> create some issues.
>
> When a function returns both error pointers and NULL then NULL is a
> special case of success.  Like say you have:  "p = start_feature();".
> If there is an allocation failure, then the code should return -ENOMEM
> and the whole thing should fail.  But if the feature is optional and
> the user has disabled it in the config then we return NULL and the
> caller has to be able to accept that.  There are a lot of these
> IS_ERR_OR_NULL() checks in the xen driver...
>
> The one here is clearly buggy because returning NULL would lead to a
> run time failure.  All these IS_ERR_OR_NULL() should be checked and
> probably changed to just IS_ERR().
>
> This sort of change is probably change is probably easiest if you build
> the Smatch DB and you can check if Smatch thinks the functions return
> NULL.
>
> ~/smatch/smatch_data/db/smdb.py return_states gem_create | grep INTERNAL
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 58 |  (-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 62 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 66 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 68 | 0,(-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
>
> Smatch thinks that gem_create() sometimes returns NULL or error pointers
> but that's because of a bug in the unreleased version of Smatch and
> because gem_create() uses IS_ERR_OR_NULL().
>
>     141
>     142          return &xen_obj->base;
>     143  }
>
> regards,
> dan carpenter

Thank you for the report, I will try to find some time to look into this 
and come up with fixes.

Could you please (probably off-list) instruct me or give any pointers to 
how to reproduce

the results of the analyzer shown above?

Thank you,

Oleksandr
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Oleksandr Andrushchenko]

On 4/21/20 13:45, Dan Carpenter wrote:
> Hi Kernel Janitors,
Hi
>
> 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);
>
> This warning is old and it's actually the result of a bug I had in my
> devel version of Smatch yesterday.  xen_obj can't really be NULL, but
> if it were then the caller would return success which would probably
> create some issues.
>
> When a function returns both error pointers and NULL then NULL is a
> special case of success.  Like say you have:  "p = start_feature();".
> If there is an allocation failure, then the code should return -ENOMEM
> and the whole thing should fail.  But if the feature is optional and
> the user has disabled it in the config then we return NULL and the
> caller has to be able to accept that.  There are a lot of these
> IS_ERR_OR_NULL() checks in the xen driver...
>
> The one here is clearly buggy because returning NULL would lead to a
> run time failure.  All these IS_ERR_OR_NULL() should be checked and
> probably changed to just IS_ERR().
>
> This sort of change is probably change is probably easiest if you build
> the Smatch DB and you can check if Smatch thinks the functions return
> NULL.
>
> ~/smatch/smatch_data/db/smdb.py return_states gem_create | grep INTERNAL
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 58 |  (-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 62 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 66 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 68 | 0,(-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
>
> Smatch thinks that gem_create() sometimes returns NULL or error pointers
> but that's because of a bug in the unreleased version of Smatch and
> because gem_create() uses IS_ERR_OR_NULL().
>
>     141
>     142          return &xen_obj->base;
>     143  }
>
> regards,
> dan carpenter

Thank you for the report, I will try to find some time to look into this 
and come up with fixes.

Could you please (probably off-list) instruct me or give any pointers to 
how to reproduce

the results of the analyzer shown above?

Thank you,

Oleksandr

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

[Dan Carpenter]

It turns out there aren't that many of these in xen.

$ grep IS_ERR_OR_NULL drivers/gpu/drm/xen/ -Rn
drivers/gpu/drm/xen/xen_drm_front_kms.c:63:     if (IS_ERR_OR_NULL(fb))
drivers/gpu/drm/xen/xen_drm_front_gem.c:86:     if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front_gem.c:120:    if (IS_ERR_OR_NULL(xen_obj->pages)) {
drivers/gpu/drm/xen/xen_drm_front_gem.c:139:    if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front_gem.c:197:    if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front.c:403:        if (IS_ERR_OR_NULL(obj)) {

They're all wrong, because if the pointer was really NULL then it's
not handled correctly and would eventually lead to a runtime failure.

Normally Smatch is smart enough to know that the pointer isn't NULL so
it doesn't generate a warning but yesterday I introduced a bug in Smatch
by mistake.  It's fixed now.

regards,
dan carpenter

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

[Dan Carpenter]

It turns out there aren't that many of these in xen.

$ grep IS_ERR_OR_NULL drivers/gpu/drm/xen/ -Rn
drivers/gpu/drm/xen/xen_drm_front_kms.c:63:     if (IS_ERR_OR_NULL(fb))
drivers/gpu/drm/xen/xen_drm_front_gem.c:86:     if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front_gem.c:120:    if (IS_ERR_OR_NULL(xen_obj->pages)) {
drivers/gpu/drm/xen/xen_drm_front_gem.c:139:    if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front_gem.c:197:    if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front.c:403:        if (IS_ERR_OR_NULL(obj)) {

They're all wrong, because if the pointer was really NULL then it's
not handled correctly and would eventually lead to a runtime failure.

Normally Smatch is smart enough to know that the pointer isn't NULL so
it doesn't generate a warning but yesterday I introduced a bug in Smatch
by mistake.  It's fixed now.

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Dan Carpenter]

It turns out there aren't that many of these in xen.

$ grep IS_ERR_OR_NULL drivers/gpu/drm/xen/ -Rn
drivers/gpu/drm/xen/xen_drm_front_kms.c:63:     if (IS_ERR_OR_NULL(fb))
drivers/gpu/drm/xen/xen_drm_front_gem.c:86:     if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front_gem.c:120:    if (IS_ERR_OR_NULL(xen_obj->pages)) {
drivers/gpu/drm/xen/xen_drm_front_gem.c:139:    if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front_gem.c:197:    if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front.c:403:        if (IS_ERR_OR_NULL(obj)) {

They're all wrong, because if the pointer was really NULL then it's
not handled correctly and would eventually lead to a runtime failure.

Normally Smatch is smart enough to know that the pointer isn't NULL so
it doesn't generate a warning but yesterday I introduced a bug in Smatch
by mistake.  It's fixed now.

regards,
dan carpenter

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

[Oleksandr Andrushchenko]

T24gNC8yMS8yMCAxNDo1MSwgRGFuIENhcnBlbnRlciB3cm90ZToNCj4gSXQgdHVybnMgb3V0IHRo
ZXJlIGFyZW4ndCB0aGF0IG1hbnkgb2YgdGhlc2UgaW4geGVuLg0KPg0KPiAkIGdyZXAgSVNfRVJS
X09SX05VTEwgZHJpdmVycy9ncHUvZHJtL3hlbi8gLVJuDQo+IGRyaXZlcnMvZ3B1L2RybS94ZW4v
eGVuX2RybV9mcm9udF9rbXMuYzo2MzogICAgIGlmIChJU19FUlJfT1JfTlVMTChmYikpDQo+IGRy
aXZlcnMvZ3B1L2RybS94ZW4veGVuX2RybV9mcm9udF9nZW0uYzo4NjogICAgIGlmIChJU19FUlJf
T1JfTlVMTCh4ZW5fb2JqKSkNCj4gZHJpdmVycy9ncHUvZHJtL3hlbi94ZW5fZHJtX2Zyb250X2dl
bS5jOjEyMDogICAgaWYgKElTX0VSUl9PUl9OVUxMKHhlbl9vYmotPnBhZ2VzKSkgew0KPiBkcml2
ZXJzL2dwdS9kcm0veGVuL3hlbl9kcm1fZnJvbnRfZ2VtLmM6MTM5OiAgICBpZiAoSVNfRVJSX09S
X05VTEwoeGVuX29iaikpDQo+IGRyaXZlcnMvZ3B1L2RybS94ZW4veGVuX2RybV9mcm9udF9nZW0u
YzoxOTc6ICAgIGlmIChJU19FUlJfT1JfTlVMTCh4ZW5fb2JqKSkNCj4gZHJpdmVycy9ncHUvZHJt
L3hlbi94ZW5fZHJtX2Zyb250LmM6NDAzOiAgICAgICAgaWYgKElTX0VSUl9PUl9OVUxMKG9iaikp
IHsNCj4NCj4gVGhleSdyZSBhbGwgd3JvbmcsIGJlY2F1c2UgaWYgdGhlIHBvaW50ZXIgd2FzIHJl
YWxseSBOVUxMIHRoZW4gaXQncw0KPiBub3QgaGFuZGxlZCBjb3JyZWN0bHkgYW5kIHdvdWxkIGV2
ZW50dWFsbHkgbGVhZCB0byBhIHJ1bnRpbWUgZmFpbHVyZS4NCj4NCj4gTm9ybWFsbHkgU21hdGNo
IGlzIHNtYXJ0IGVub3VnaCB0byBrbm93IHRoYXQgdGhlIHBvaW50ZXIgaXNuJ3QgTlVMTCBzbw0K
PiBpdCBkb2Vzbid0IGdlbmVyYXRlIGEgd2FybmluZyBidXQgeWVzdGVyZGF5IEkgaW50cm9kdWNl
ZCBhIGJ1ZyBpbiBTbWF0Y2gNCj4gYnkgbWlzdGFrZS4gIEl0J3MgZml4ZWQgbm93Lg0KPg0KPiBy
ZWdhcmRzLA0KPiBkYW4gY2FycGVudGVyDQo+DQpUaGFuayB5b3UsDQoNCkknbGwgaGF2ZSBhIGxv
b2sgYXQgdGhvc2UNCg=

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

[Oleksandr Andrushchenko]

On 4/21/20 14:51, Dan Carpenter wrote:
> It turns out there aren't that many of these in xen.
>
> $ grep IS_ERR_OR_NULL drivers/gpu/drm/xen/ -Rn
> drivers/gpu/drm/xen/xen_drm_front_kms.c:63:     if (IS_ERR_OR_NULL(fb))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:86:     if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:120:    if (IS_ERR_OR_NULL(xen_obj->pages)) {
> drivers/gpu/drm/xen/xen_drm_front_gem.c:139:    if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:197:    if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front.c:403:        if (IS_ERR_OR_NULL(obj)) {
>
> They're all wrong, because if the pointer was really NULL then it's
> not handled correctly and would eventually lead to a runtime failure.
>
> Normally Smatch is smart enough to know that the pointer isn't NULL so
> it doesn't generate a warning but yesterday I introduced a bug in Smatch
> by mistake.  It's fixed now.
>
> regards,
> dan carpenter
>
Thank you,

I'll have a look at those
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Oleksandr Andrushchenko]

On 4/21/20 14:51, Dan Carpenter wrote:
> It turns out there aren't that many of these in xen.
>
> $ grep IS_ERR_OR_NULL drivers/gpu/drm/xen/ -Rn
> drivers/gpu/drm/xen/xen_drm_front_kms.c:63:     if (IS_ERR_OR_NULL(fb))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:86:     if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:120:    if (IS_ERR_OR_NULL(xen_obj->pages)) {
> drivers/gpu/drm/xen/xen_drm_front_gem.c:139:    if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:197:    if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front.c:403:        if (IS_ERR_OR_NULL(obj)) {
>
> They're all wrong, because if the pointer was really NULL then it's
> not handled correctly and would eventually lead to a runtime failure.
>
> Normally Smatch is smart enough to know that the pointer isn't NULL so
> it doesn't generate a warning but yesterday I introduced a bug in Smatch
> by mistake.  It's fixed now.
>
> regards,
> dan carpenter
>
Thank you,

I'll have a look at those

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

[Julia Lawall]

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.

julia

>
> This warning is old and it's actually the result of a bug I had in my
> devel version of Smatch yesterday.  xen_obj can't really be NULL, but
> if it were then the caller would return success which would probably
> create some issues.
>
> When a function returns both error pointers and NULL then NULL is a
> special case of success.  Like say you have:  "p = start_feature();".
> If there is an allocation failure, then the code should return -ENOMEM
> and the whole thing should fail.  But if the feature is optional and
> the user has disabled it in the config then we return NULL and the
> caller has to be able to accept that.  There are a lot of these
> IS_ERR_OR_NULL() checks in the xen driver...
>
> The one here is clearly buggy because returning NULL would lead to a
> run time failure.  All these IS_ERR_OR_NULL() should be checked and
> probably changed to just IS_ERR().
>
> This sort of change is probably change is probably easiest if you build
> the Smatch DB and you can check if Smatch thinks the functions return
> NULL.
>
> ~/smatch/smatch_data/db/smdb.py return_states gem_create | grep INTERNAL
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 58 |  (-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 62 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 66 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 68 | 0,(-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
>
> Smatch thinks that gem_create() sometimes returns NULL or error pointers
> but that's because of a bug in the unreleased version of Smatch and
> because gem_create() uses IS_ERR_OR_NULL().
>
>    141
>    142          return &xen_obj->base;
>    143  }
>
> regards,
> dan carpenter
>

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

[Julia Lawall]

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.

julia

>
> This warning is old and it's actually the result of a bug I had in my
> devel version of Smatch yesterday.  xen_obj can't really be NULL, but
> if it were then the caller would return success which would probably
> create some issues.
>
> When a function returns both error pointers and NULL then NULL is a
> special case of success.  Like say you have:  "p = start_feature();".
> If there is an allocation failure, then the code should return -ENOMEM
> and the whole thing should fail.  But if the feature is optional and
> the user has disabled it in the config then we return NULL and the
> caller has to be able to accept that.  There are a lot of these
> IS_ERR_OR_NULL() checks in the xen driver...
>
> The one here is clearly buggy because returning NULL would lead to a
> run time failure.  All these IS_ERR_OR_NULL() should be checked and
> probably changed to just IS_ERR().
>
> This sort of change is probably change is probably easiest if you build
> the Smatch DB and you can check if Smatch thinks the functions return
> NULL.
>
> ~/smatch/smatch_data/db/smdb.py return_states gem_create | grep INTERNAL
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 58 |  (-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 62 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 66 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 68 | 0,(-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
>
> Smatch thinks that gem_create() sometimes returns NULL or error pointers
> but that's because of a bug in the unreleased version of Smatch and
> because gem_create() uses IS_ERR_OR_NULL().
>
>    141
>    142          return &xen_obj->base;
>    143  }
>
> regards,
> dan carpenter
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Julia Lawall]

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.

julia

>
> This warning is old and it's actually the result of a bug I had in my
> devel version of Smatch yesterday.  xen_obj can't really be NULL, but
> if it were then the caller would return success which would probably
> create some issues.
>
> When a function returns both error pointers and NULL then NULL is a
> special case of success.  Like say you have:  "p = start_feature();".
> If there is an allocation failure, then the code should return -ENOMEM
> and the whole thing should fail.  But if the feature is optional and
> the user has disabled it in the config then we return NULL and the
> caller has to be able to accept that.  There are a lot of these
> IS_ERR_OR_NULL() checks in the xen driver...
>
> The one here is clearly buggy because returning NULL would lead to a
> run time failure.  All these IS_ERR_OR_NULL() should be checked and
> probably changed to just IS_ERR().
>
> This sort of change is probably change is probably easiest if you build
> the Smatch DB and you can check if Smatch thinks the functions return
> NULL.
>
> ~/smatch/smatch_data/db/smdb.py return_states gem_create | grep INTERNAL
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 58 |  (-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 62 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 66 | 4065035897849303040 |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 68 | 0,(-4095)-(-1) |      INTERNAL |  -1 |                      | struct xen_gem_object*(*)(struct drm_device*, ulong) |
>
> Smatch thinks that gem_create() sometimes returns NULL or error pointers
> but that's because of a bug in the unreleased version of Smatch and
> because gem_create() uses IS_ERR_OR_NULL().
>
>    141
>    142          return &xen_obj->base;
>    143  }
>
> regards,
> dan carpenter
>

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

[Dan Carpenter]

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.

regards,
dan carpenter

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

[Dan Carpenter]

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.

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Dan Carpenter]

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.

regards,
dan carpenter

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

[Julia Lawall]

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:

drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c in create_udp_flow
fs/overlayfs/namei.c in ovl_index_upper
sound/soc/qcom/qdsp6/q6adm.c in q6adm_open
drivers/clk/clk.c in clk_hw_create_clk

julia

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

[Julia Lawall]

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:

drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c in create_udp_flow
fs/overlayfs/namei.c in ovl_index_upper
sound/soc/qcom/qdsp6/q6adm.c in q6adm_open
drivers/clk/clk.c in clk_hw_create_clk

julia
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Julia Lawall]

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:

drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c in create_udp_flow
fs/overlayfs/namei.c in ovl_index_upper
sound/soc/qcom/qdsp6/q6adm.c in q6adm_open
drivers/clk/clk.c in clk_hw_create_clk

julia

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

[Dan Carpenter]

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

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

[Dan Carpenter]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Dan Carpenter]

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

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

[Oleksandr Andrushchenko]

DQpPbiA0LzIxLzIwIDI6NTEgUE0sIERhbiBDYXJwZW50ZXIgd3JvdGU6DQo+IEl0IHR1cm5zIG91
dCB0aGVyZSBhcmVuJ3QgdGhhdCBtYW55IG9mIHRoZXNlIGluIHhlbi4NCj4NCj4gJCBncmVwIElT
X0VSUl9PUl9OVUxMIGRyaXZlcnMvZ3B1L2RybS94ZW4vIC1Sbg0KPiBkcml2ZXJzL2dwdS9kcm0v
eGVuL3hlbl9kcm1fZnJvbnRfa21zLmM6NjM6ICAgICBpZiAoSVNfRVJSX09SX05VTEwoZmIpKQ0K
PiBkcml2ZXJzL2dwdS9kcm0veGVuL3hlbl9kcm1fZnJvbnRfZ2VtLmM6ODY6ICAgICBpZiAoSVNf
RVJSX09SX05VTEwoeGVuX29iaikpDQo+IGRyaXZlcnMvZ3B1L2RybS94ZW4veGVuX2RybV9mcm9u
dF9nZW0uYzoxMjA6ICAgIGlmIChJU19FUlJfT1JfTlVMTCh4ZW5fb2JqLT5wYWdlcykpIHsNCj4g
ZHJpdmVycy9ncHUvZHJtL3hlbi94ZW5fZHJtX2Zyb250X2dlbS5jOjEzOTogICAgaWYgKElTX0VS
Ul9PUl9OVUxMKHhlbl9vYmopKQ0KPiBkcml2ZXJzL2dwdS9kcm0veGVuL3hlbl9kcm1fZnJvbnRf
Z2VtLmM6MTk3OiAgICBpZiAoSVNfRVJSX09SX05VTEwoeGVuX29iaikpDQo+IGRyaXZlcnMvZ3B1
L2RybS94ZW4veGVuX2RybV9mcm9udC5jOjQwMzogICAgICAgIGlmIChJU19FUlJfT1JfTlVMTChv
YmopKSB7DQo+DQo+IFRoZXkncmUgYWxsIHdyb25nLCBiZWNhdXNlIGlmIHRoZSBwb2ludGVyIHdh
cyByZWFsbHkgTlVMTCB0aGVuIGl0J3MNCj4gbm90IGhhbmRsZWQgY29ycmVjdGx5IGFuZCB3b3Vs
ZCBldmVudHVhbGx5IGxlYWQgdG8gYSBydW50aW1lIGZhaWx1cmUuDQoNCkl0IHNlZW1zIHRoYXQg
eW91IHdlcmUgcmlnaHQgYW5kIEkgY2FuIHNpbXBseSBjaGFuZ2UgYWxsIElTX0VSUl9PUl9OVUxM
IA0KdG8ganVzdCBJU19FUlINCg0KSSBhbSBwbGFubmluZyBhIHNlcmllcyBvZiBwYXRjaGVzIGxh
dGVyIG9uLCBzbyBJJ2xsIGluY2x1ZGUgdGhpcyBmaXggYXMgd2VsbA0KDQo+DQo+IE5vcm1hbGx5
IFNtYXRjaCBpcyBzbWFydCBlbm91Z2ggdG8ga25vdyB0aGF0IHRoZSBwb2ludGVyIGlzbid0IE5V
TEwgc28NCj4gaXQgZG9lc24ndCBnZW5lcmF0ZSBhIHdhcm5pbmcgYnV0IHllc3RlcmRheSBJIGlu
dHJvZHVjZWQgYSBidWcgaW4gU21hdGNoDQo+IGJ5IG1pc3Rha2UuICBJdCdzIGZpeGVkIG5vdy4N
Cj4NCj4gcmVnYXJkcywNCj4gZGFuIGNhcnBlbnRlcg0KPg0KVGhhbmsgeW91LA0KDQpPbGVrc2Fu
ZHINCg=

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

[Oleksandr Andrushchenko]

On 4/21/20 2:51 PM, Dan Carpenter wrote:
> It turns out there aren't that many of these in xen.
>
> $ grep IS_ERR_OR_NULL drivers/gpu/drm/xen/ -Rn
> drivers/gpu/drm/xen/xen_drm_front_kms.c:63:     if (IS_ERR_OR_NULL(fb))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:86:     if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:120:    if (IS_ERR_OR_NULL(xen_obj->pages)) {
> drivers/gpu/drm/xen/xen_drm_front_gem.c:139:    if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:197:    if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front.c:403:        if (IS_ERR_OR_NULL(obj)) {
>
> They're all wrong, because if the pointer was really NULL then it's
> not handled correctly and would eventually lead to a runtime failure.

It seems that you were right and I can simply change all IS_ERR_OR_NULL 
to just IS_ERR

I am planning a series of patches later on, so I'll include this fix as well

>
> Normally Smatch is smart enough to know that the pointer isn't NULL so
> it doesn't generate a warning but yesterday I introduced a bug in Smatch
> by mistake.  It's fixed now.
>
> regards,
> dan carpenter
>
Thank you,

Oleksandr
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Oleksandr Andrushchenko]

On 4/21/20 2:51 PM, Dan Carpenter wrote:
> It turns out there aren't that many of these in xen.
>
> $ grep IS_ERR_OR_NULL drivers/gpu/drm/xen/ -Rn
> drivers/gpu/drm/xen/xen_drm_front_kms.c:63:     if (IS_ERR_OR_NULL(fb))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:86:     if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:120:    if (IS_ERR_OR_NULL(xen_obj->pages)) {
> drivers/gpu/drm/xen/xen_drm_front_gem.c:139:    if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:197:    if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front.c:403:        if (IS_ERR_OR_NULL(obj)) {
>
> They're all wrong, because if the pointer was really NULL then it's
> not handled correctly and would eventually lead to a runtime failure.

It seems that you were right and I can simply change all IS_ERR_OR_NULL 
to just IS_ERR

I am planning a series of patches later on, so I'll include this fix as well

>
> Normally Smatch is smart enough to know that the pointer isn't NULL so
> it doesn't generate a warning but yesterday I introduced a bug in Smatch
> by mistake.  It's fixed now.
>
> regards,
> dan carpenter
>
Thank you,

Oleksandr