lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 26 May 2022 19:00:06 +0200 From: Eugenio Perez Martin <eperezma@...hat.com> To: Dan Carpenter <dan.carpenter@...cle.com> Cc: Stefano Garzarella <sgarzare@...hat.com>, "Dawar, Gautam" <gautam.dawar@....com>, "Michael S. Tsirkin" <mst@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "virtualization@...ts.linux-foundation.org" <virtualization@...ts.linux-foundation.org>, Jason Wang <jasowang@...hat.com>, Zhu Lingshan <lingshan.zhu@...el.com>, "martinh@...inx.com" <martinh@...inx.com>, "ecree.xilinx@...il.com" <ecree.xilinx@...il.com>, Eli Cohen <elic@...dia.com>, Parav Pandit <parav@...dia.com>, Wu Zongyong <wuzongyong@...ux.alibaba.com>, "dinang@...inx.com" <dinang@...inx.com>, Christophe JAILLET <christophe.jaillet@...adoo.fr>, Xie Yongji <xieyongji@...edance.com>, "lulu@...hat.com" <lulu@...hat.com>, "martinpo@...inx.com" <martinpo@...inx.com>, "pabloc@...inx.com" <pabloc@...inx.com>, Longpeng <longpeng2@...wei.com>, "Piotr.Uminski@...el.com" <Piotr.Uminski@...el.com>, "Kamde, Tanuj" <tanuj.kamde@....com>, Si-Wei Liu <si-wei.liu@...cle.com>, "habetsm.xilinx@...il.com" <habetsm.xilinx@...il.com>, "lvivier@...hat.com" <lvivier@...hat.com>, Zhang Min <zhang.min9@....com.cn>, "hanand@...inx.com" <hanand@...inx.com> Subject: Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit On Thu, May 26, 2022 at 3:21 PM Dan Carpenter <dan.carpenter@...cle.com> wrote: > > On Thu, May 26, 2022 at 02:44:02PM +0200, Eugenio Perez Martin wrote: > > > >> +static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v) { > > > >> + struct vdpa_device *vdpa = v->vdpa; > > > >> + const struct vdpa_config_ops *ops = vdpa->config; > > > >> + > > > >> + return ops->stop; > > > >> [GD>>] Would it be better to explicitly return a bool to match the return type? > > > > > > > >I'm not sure about the kernel code style regarding that casting. Maybe > > > >it's better to return !!ops->stop here. The macros likely and unlikely > > > >do that. > > > > > > IIUC `ops->stop` is a function pointer, so what about > > > > > > return ops->stop != NULL; > > > > > > > I'm ok with any method proposed. Both three ways can be found in the > > kernel so I think they are all valid (although the double negation is > > from bool to integer in (0,1) set actually). > > > > Maybe Jason or Michael (as maintainers) can state the preferred method here. > > Always just do whatever the person who responded feels like because > they're likely the person who cares the most. ;) > This is interesting and I think it's good advice :). I'm fine with whatever we chose, I just wanted to "break the tie" between the three. > I don't think there are any static analysis tools which will complain > about this. Smatch will complain if you return a negative literal. Maybe a negative literal is a bad code signal, yes. > It feels like returning any literal that isn't 1 or 0 should trigger a > warning... I've written that and will check it out tonight. > I'm not sure this should be so strict, or "literal" does not include pointers? As an experiment, can Smatch be used to count how many times a returned pointer is converted to int / bool before returning vs not converted? I find Smatch interesting, especially when switching between projects frequently. Does it support changing the code like clang-format? To offload cognitive load to tools is usually good :). Thanks! > Really anything negative should trigger a warning. See new Smatch stuff > below. > > regards, > dan carpenter > > ================ TEST CASE ========================= > > int x; > _Bool one(int *p) > { > if (p) > x = -2; > return x; > } > _Bool two(int *p) > { > return -4; // returning 2 triggers a warning now > } > > =============== OUTPUT ============================= > > test.c:10 one() warn: potential negative cast to bool 'x' > test.c:14 two() warn: signedness bug returning '(-4)' > test.c:14 two() warn: '(-4)' is not bool > > =============== CODE =============================== > > #include "smatch.h" > #include "smatch_extra.h" > #include "smatch_slist.h" > > static int my_id; > > static void match_literals(struct expression *ret_value) > { > struct symbol *type; > sval_t sval; > > type = cur_func_return_type(); > if (!type || sval_type_max(type).value != 1) > return; > > if (!get_implied_value(ret_value, &sval)) > return; > > if (sval.value == 0 || sval.value == 1) > return; > > sm_warning("'%s' is not bool", sval_to_str(sval)); > } > > static void match_any_negative(struct expression *ret_value) > { > struct symbol *type; > struct sm_state *extra, *sm; > sval_t sval; > char *name; > > type = cur_func_return_type(); > if (!type || sval_type_max(type).value != 1) > return; > > extra = get_extra_sm_state(ret_value); > if (!extra) > return; > FOR_EACH_PTR(extra->possible, sm) { > if (estate_get_single_value(sm->state, &sval) && > sval_is_negative(sval)) { > name = expr_to_str(ret_value); > sm_warning("potential negative cast to bool '%s'", name); > free_string(name); > return; > } > } END_FOR_EACH_PTR(sm); > } > > void check_bool_return(int id) > { > my_id = id; > > add_hook(&match_literals, RETURN_HOOK); > add_hook(&match_any_negative, RETURN_HOOK); > } >
Powered by blists - more mailing lists