Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

[Stefano Garzarella]

On Thu, May 26, 2022 at 10:57:03AM +0200, Eugenio Perez Martin wrote:
>On Wed, May 25, 2022 at 1:23 PM Dawar, Gautam <gautam.dawar@amd.com> wrote:
>>
>> [AMD Official Use Only - General]
>>
>> -----Original Message-----
>> From: Eugenio Pérez <eperezma@redhat.com>
>> Sent: Wednesday, May 25, 2022 4:29 PM
>> To: Michael S. Tsirkin <mst@redhat.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org; virtualization@lists.linux-foundation.org; Jason Wang <jasowang@redhat.com>
>> Cc: Zhu Lingshan <lingshan.zhu@intel.com>; martinh@xilinx.com; Stefano Garzarella <sgarzare@redhat.com>; ecree.xilinx@gmail.com; Eli Cohen <elic@nvidia.com>; Dan Carpenter <dan.carpenter@oracle.com>; Parav Pandit <parav@nvidia.com>; Wu Zongyong <wuzongyong@linux.alibaba.com>; dinang@xilinx.com; Christophe JAILLET <christophe.jaillet@wanadoo.fr>; Xie Yongji <xieyongji@bytedance.com>; Dawar, Gautam <gautam.dawar@amd.com>; lulu@redhat.com; martinpo@xilinx.com; pabloc@xilinx.com; Longpeng <longpeng2@huawei.com>; Piotr.Uminski@intel.com; Kamde, Tanuj <tanuj.kamde@amd.com>; Si-Wei Liu <si-wei.liu@oracle.com>; habetsm.xilinx@gmail.com; lvivier@redhat.com; Zhang Min <zhang.min9@zte.com.cn>; hanand@xilinx.com
>> Subject: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit
>>
>> [CAUTION: External Email]
>>
>> Userland knows if it can stop the device or not by checking this feature bit.
>>
>> It's only offered if the vdpa driver backend implements the stop() operation callback, and try to set it if the backend does not offer that callback is an error.
>>
>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> ---
>>  drivers/vhost/vdpa.c             | 16 +++++++++++++++-
>>  include/uapi/linux/vhost_types.h |  2 ++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 1f1d1c425573..32713db5831d 100644
>> --- a/drivers/vhost/vdpa.c
>> +++ b/drivers/vhost/vdpa.c
>> @@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
>>         return 0;
>>  }
>>
>> +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;

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

[Dan Carpenter]

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

I don't think there are any static analysis tools which will complain
about this.  Smatch will complain if you return a negative literal.
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.

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

[Dan Carpenter]

On Thu, May 26, 2022 at 07:00:06PM +0200, Eugenio Perez Martin wrote:
> > 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?
> 

What I mean in exact terms, is that if you're returning a known value
and the function returns bool then the known value should be 0 or 1.
Don't "return 3;".  This new warning will complain if you return a known
pointer as in "return &a;".  It won't complain if you return an
unknown pointer "return p;".

> 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'm not super excited to write that code...  :/

> 
> 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 :).

No.  Coccinelle does that really well though.

regards,
dan carpenter

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

[Dan Carpenter]

On Fri, May 27, 2022 at 08:50:16AM +0200, Eugenio Perez Martin wrote:
> On Thu, May 26, 2022 at 9:07 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Thu, May 26, 2022 at 07:00:06PM +0200, Eugenio Perez Martin wrote:
> > > > 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?
> > >
> >
> > What I mean in exact terms, is that if you're returning a known value
> > and the function returns bool then the known value should be 0 or 1.
> > Don't "return 3;".  This new warning will complain if you return a known
> > pointer as in "return &a;".  It won't complain if you return an
> > unknown pointer "return p;".
> >
> 
> Ok, thanks for the clarification.
> 
> > > 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'm not super excited to write that code...  :/
> >
> 
> Sure, I understand. I meant if it was possible or if that is too far
> beyond its scope.

To be honest, I misread what you were asking.  GCC won't let you return
a pointer with an implied cast to int.  It has to be explicit.  So there
are zero of those.  It's not hard to look for pointers with an implied
cast to bool.

static void match_pointer(struct expression *ret_value)
{
        struct symbol *type;
        char *name;

        type = cur_func_return_type();
        if (!type || sval_type_max(type).value != 1)
                return;

        if (!is_pointer(ret_value))
                return;

        name = expr_to_str(ret_value);
        sm_msg("'%s' return pointer cast to bool", name);
        free_string(name);
}

regards,
dan carpenter

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

[Dan Carpenter]

On Fri, May 27, 2022 at 10:36:55AM +0300, Dan Carpenter wrote:
> static void match_pointer(struct expression *ret_value)
> {
>         struct symbol *type;
>         char *name;
> 
>         type = cur_func_return_type();
>         if (!type || sval_type_max(type).value != 1)
>                 return;
> 
>         if (!is_pointer(ret_value))
>                 return;
> 
>         name = expr_to_str(ret_value);
>         sm_msg("'%s' return pointer cast to bool", name);
>         free_string(name);
> }

I found a bug in Smatch with this check.  With the Smatch bug fixed then
there is only only place which does a legitimate implied pointer to bool
cast.  A different function returns NULL on error instead of false.

drivers/gpu/drm/i915/display/intel_dmc.c:249 intel_dmc_has_payload() 'i915->dmc.dmc_info[0]->payload' return pointer cast to bool
drivers/net/wireless/rndis_wlan.c:1980 rndis_bss_info_update() '(0)' return pointer cast to bool
drivers/net/wireless/rndis_wlan.c:1989 rndis_bss_info_update() '(0)' return pointer cast to bool
drivers/net/wireless/rndis_wlan.c:1995 rndis_bss_info_update() '(0)' return pointer cast to bool

regards,
dan carpenter

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

[Dan Carpenter]

On Mon, May 30, 2022 at 05:27:25PM +0300, Dan Carpenter wrote:
> On Fri, May 27, 2022 at 10:36:55AM +0300, Dan Carpenter wrote:
> > static void match_pointer(struct expression *ret_value)
> > {
> >         struct symbol *type;
> >         char *name;
> > 
> >         type = cur_func_return_type();
> >         if (!type || sval_type_max(type).value != 1)
> >                 return;
> > 
> >         if (!is_pointer(ret_value))
> >                 return;
> > 
> >         name = expr_to_str(ret_value);
> >         sm_msg("'%s' return pointer cast to bool", name);
> >         free_string(name);
> > }
> 
> I found a bug in Smatch with this check.  With the Smatch bug fixed then
> there is only only place which does a legitimate implied pointer to bool
> cast.  A different function returns NULL on error instead of false.
> 
> drivers/gpu/drm/i915/display/intel_dmc.c:249 intel_dmc_has_payload() 'i915->dmc.dmc_info[0]->payload' return pointer cast to bool
> drivers/net/wireless/rndis_wlan.c:1980 rndis_bss_info_update() '(0)' return pointer cast to bool
> drivers/net/wireless/rndis_wlan.c:1989 rndis_bss_info_update() '(0)' return pointer cast to bool
> drivers/net/wireless/rndis_wlan.c:1995 rndis_bss_info_update() '(0)' return pointer cast to bool

Hm...  I found another.  I'm not sure why it wasn't showing up before.

lib/assoc_array.c:941 assoc_array_insert_mid_shortcut() 'edit' return pointer cast to bool

That's weird.  I will re-run the test.

regards,
dan carpenter

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization