* [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 10:45 ` Dan Carpenter 0 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 10:45 UTC (permalink / raw) To: oleksandr_andrushchenko; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 10:45 ` Dan Carpenter 0 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 10:45 UTC (permalink / raw) To: oleksandr_andrushchenko; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 10:45 ` Dan Carpenter 0 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 10:45 UTC (permalink / raw) To: oleksandr_andrushchenko; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend 2020-04-21 10:45 ` Dan Carpenter (?) @ 2020-04-21 11:34 ` Oleksandr Andrushchenko -1 siblings, 0 replies; 27+ messages in thread From: Oleksandr Andrushchenko @ 2020-04-21 11:34 UTC (permalink / raw) To: Dan Carpenter; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 11:34 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 27+ messages in thread From: Oleksandr Andrushchenko @ 2020-04-21 11:34 UTC (permalink / raw) To: Dan Carpenter; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 11:34 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 27+ messages in thread From: Oleksandr Andrushchenko @ 2020-04-21 11:34 UTC (permalink / raw) To: Dan Carpenter; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend 2020-04-21 11:34 ` Oleksandr Andrushchenko (?) @ 2020-04-21 11:51 ` Dan Carpenter -1 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 11:51 UTC (permalink / raw) To: Oleksandr Andrushchenko; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 11:51 ` Dan Carpenter 0 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 11:51 UTC (permalink / raw) To: Oleksandr Andrushchenko; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 11:51 ` Dan Carpenter 0 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 11:51 UTC (permalink / raw) To: Oleksandr Andrushchenko; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend 2020-04-21 11:51 ` Dan Carpenter (?) @ 2020-04-21 12:54 ` Oleksandr Andrushchenko -1 siblings, 0 replies; 27+ messages in thread From: Oleksandr Andrushchenko @ 2020-04-21 12:54 UTC (permalink / raw) To: Dan Carpenter; +Cc: xen-devel, kernel-janitors, dri-devel T24gNC8yMS8yMCAxNDo1MSwgRGFuIENhcnBlbnRlciB3cm90ZToNCj4gSXQgdHVybnMgb3V0IHRo ZXJlIGFyZW4ndCB0aGF0IG1hbnkgb2YgdGhlc2UgaW4geGVuLg0KPg0KPiAkIGdyZXAgSVNfRVJS X09SX05VTEwgZHJpdmVycy9ncHUvZHJtL3hlbi8gLVJuDQo+IGRyaXZlcnMvZ3B1L2RybS94ZW4v eGVuX2RybV9mcm9udF9rbXMuYzo2MzogICAgIGlmIChJU19FUlJfT1JfTlVMTChmYikpDQo+IGRy aXZlcnMvZ3B1L2RybS94ZW4veGVuX2RybV9mcm9udF9nZW0uYzo4NjogICAgIGlmIChJU19FUlJf T1JfTlVMTCh4ZW5fb2JqKSkNCj4gZHJpdmVycy9ncHUvZHJtL3hlbi94ZW5fZHJtX2Zyb250X2dl bS5jOjEyMDogICAgaWYgKElTX0VSUl9PUl9OVUxMKHhlbl9vYmotPnBhZ2VzKSkgew0KPiBkcml2 ZXJzL2dwdS9kcm0veGVuL3hlbl9kcm1fZnJvbnRfZ2VtLmM6MTM5OiAgICBpZiAoSVNfRVJSX09S X05VTEwoeGVuX29iaikpDQo+IGRyaXZlcnMvZ3B1L2RybS94ZW4veGVuX2RybV9mcm9udF9nZW0u YzoxOTc6ICAgIGlmIChJU19FUlJfT1JfTlVMTCh4ZW5fb2JqKSkNCj4gZHJpdmVycy9ncHUvZHJt L3hlbi94ZW5fZHJtX2Zyb250LmM6NDAzOiAgICAgICAgaWYgKElTX0VSUl9PUl9OVUxMKG9iaikp IHsNCj4NCj4gVGhleSdyZSBhbGwgd3JvbmcsIGJlY2F1c2UgaWYgdGhlIHBvaW50ZXIgd2FzIHJl YWxseSBOVUxMIHRoZW4gaXQncw0KPiBub3QgaGFuZGxlZCBjb3JyZWN0bHkgYW5kIHdvdWxkIGV2 ZW50dWFsbHkgbGVhZCB0byBhIHJ1bnRpbWUgZmFpbHVyZS4NCj4NCj4gTm9ybWFsbHkgU21hdGNo IGlzIHNtYXJ0IGVub3VnaCB0byBrbm93IHRoYXQgdGhlIHBvaW50ZXIgaXNuJ3QgTlVMTCBzbw0K PiBpdCBkb2Vzbid0IGdlbmVyYXRlIGEgd2FybmluZyBidXQgeWVzdGVyZGF5IEkgaW50cm9kdWNl ZCBhIGJ1ZyBpbiBTbWF0Y2gNCj4gYnkgbWlzdGFrZS4gIEl0J3MgZml4ZWQgbm93Lg0KPg0KPiBy ZWdhcmRzLA0KPiBkYW4gY2FycGVudGVyDQo+DQpUaGFuayB5b3UsDQoNCkknbGwgaGF2ZSBhIGxv b2sgYXQgdGhvc2UNCg= ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 12:54 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 27+ messages in thread From: Oleksandr Andrushchenko @ 2020-04-21 12:54 UTC (permalink / raw) To: Dan Carpenter; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 12:54 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 27+ messages in thread From: Oleksandr Andrushchenko @ 2020-04-21 12:54 UTC (permalink / raw) To: Dan Carpenter; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend 2020-04-21 11:51 ` Dan Carpenter (?) @ 2020-05-08 7:34 ` Oleksandr Andrushchenko -1 siblings, 0 replies; 27+ messages in thread From: Oleksandr Andrushchenko @ 2020-05-08 7:34 UTC (permalink / raw) To: Dan Carpenter; +Cc: xen-devel, kernel-janitors, dri-devel 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= ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-05-08 7:34 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 27+ messages in thread From: Oleksandr Andrushchenko @ 2020-05-08 7:34 UTC (permalink / raw) To: Dan Carpenter; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-05-08 7:34 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 27+ messages in thread From: Oleksandr Andrushchenko @ 2020-05-08 7:34 UTC (permalink / raw) To: Dan Carpenter; +Cc: xen-devel, kernel-janitors, dri-devel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend 2020-04-21 10:45 ` Dan Carpenter (?) @ 2020-04-21 15:29 ` Julia Lawall -1 siblings, 0 replies; 27+ messages in thread From: Julia Lawall @ 2020-04-21 15:29 UTC (permalink / raw) To: Dan Carpenter Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 15:29 ` Julia Lawall 0 siblings, 0 replies; 27+ messages in thread From: Julia Lawall @ 2020-04-21 15:29 UTC (permalink / raw) To: Dan Carpenter Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 15:29 ` Julia Lawall 0 siblings, 0 replies; 27+ messages in thread From: Julia Lawall @ 2020-04-21 15:29 UTC (permalink / raw) To: Dan Carpenter Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend 2020-04-21 15:29 ` Julia Lawall (?) @ 2020-04-21 17:52 ` Dan Carpenter -1 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 17:52 UTC (permalink / raw) To: Julia Lawall Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 17:52 ` Dan Carpenter 0 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 17:52 UTC (permalink / raw) To: Julia Lawall Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 17:52 ` Dan Carpenter 0 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 17:52 UTC (permalink / raw) To: Julia Lawall Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend 2020-04-21 17:52 ` Dan Carpenter (?) @ 2020-04-21 18:59 ` Julia Lawall -1 siblings, 0 replies; 27+ messages in thread From: Julia Lawall @ 2020-04-21 18:59 UTC (permalink / raw) To: Dan Carpenter Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 18:59 ` Julia Lawall 0 siblings, 0 replies; 27+ messages in thread From: Julia Lawall @ 2020-04-21 18:59 UTC (permalink / raw) To: Dan Carpenter Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 18:59 ` Julia Lawall 0 siblings, 0 replies; 27+ messages in thread From: Julia Lawall @ 2020-04-21 18:59 UTC (permalink / raw) To: Dan Carpenter Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend 2020-04-21 18:59 ` Julia Lawall (?) @ 2020-04-21 19:33 ` Dan Carpenter -1 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 19:33 UTC (permalink / raw) To: Julia Lawall Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 19:33 ` Dan Carpenter 0 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 19:33 UTC (permalink / raw) To: Julia Lawall Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [bug report] drm/xen-front: Add support for Xen PV display frontend @ 2020-04-21 19:33 ` Dan Carpenter 0 siblings, 0 replies; 27+ messages in thread From: Dan Carpenter @ 2020-04-21 19:33 UTC (permalink / raw) To: Julia Lawall Cc: xen-devel, kernel-janitors, dri-devel, oleksandr_andrushchenko 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-05-08 8:47 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-21 10:45 [bug report] drm/xen-front: Add support for Xen PV display frontend Dan Carpenter 2020-04-21 10:45 ` Dan Carpenter 2020-04-21 10:45 ` Dan Carpenter 2020-04-21 11:34 ` Oleksandr Andrushchenko 2020-04-21 11:34 ` Oleksandr Andrushchenko 2020-04-21 11:34 ` Oleksandr Andrushchenko 2020-04-21 11:51 ` Dan Carpenter 2020-04-21 11:51 ` Dan Carpenter 2020-04-21 11:51 ` Dan Carpenter 2020-04-21 12:54 ` Oleksandr Andrushchenko 2020-04-21 12:54 ` Oleksandr Andrushchenko 2020-04-21 12:54 ` Oleksandr Andrushchenko 2020-05-08 7:34 ` Oleksandr Andrushchenko 2020-05-08 7:34 ` Oleksandr Andrushchenko 2020-05-08 7:34 ` Oleksandr Andrushchenko 2020-04-21 15:29 ` Julia Lawall 2020-04-21 15:29 ` Julia Lawall 2020-04-21 15:29 ` Julia Lawall 2020-04-21 17:52 ` Dan Carpenter 2020-04-21 17:52 ` Dan Carpenter 2020-04-21 17:52 ` Dan Carpenter 2020-04-21 18:59 ` Julia Lawall 2020-04-21 18:59 ` Julia Lawall 2020-04-21 18:59 ` Julia Lawall 2020-04-21 19:33 ` Dan Carpenter 2020-04-21 19:33 ` Dan Carpenter 2020-04-21 19:33 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.