linux-kernel - Re: [PATCH v2 1/3] drivers: hv: vmbus: Introduce latency testing

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190823033850.GA41496@Test-Virtual-Machine>
Date:   Thu, 22 Aug 2019 23:38:50 -0400
From:   Branden Bonaby <brandonbonaby94@...il.com>
To:     Michael Kelley <mikelley@...rosoft.com>
Cc:     KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "sashal@...nel.org" <sashal@...nel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] drivers: hv: vmbus: Introduce latency testing

> >  endmenu
> > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> > index 09829e15d4a0..c9c63a4033cd 100644
> > --- a/drivers/hv/connection.c
> > +++ b/drivers/hv/connection.c
> > @@ -357,6 +357,9 @@ void vmbus_on_event(unsigned long data)
> > 
> >  	trace_vmbus_on_event(channel);
> > 
> > +#ifdef CONFIG_HYPERV_TESTING
> > +	hv_debug_delay_test(channel, INTERRUPT_DELAY);
> > +#endif /* CONFIG_HYPERV_TESTING */
> 
> You are following Vitaly's suggestion to use #ifdef's so no code is
> generated when HYPERV_TESTING is not enabled.  However, this
> direct approach to using #ifdef's really clutters the code and makes
> it harder to read and follow.  The better approach is to use the
> #ifdef in the include file where the functions are defined.  If
> HYPERV_TESTING is not enabled, provide a #else that defines
> the function with an empty implementation for which the compiler
> will generate no code.   An as example, see the function definition
> for hyperv_init() in arch/x86/include/asm/mshyperv.h.  There are
> several functions treated similarly in that include file.
> 

I checked out the code in arch/x86/include/asm/mshyperv.h, after
thinking about it, I'm wondering if it would be better just to have
two files one called hv_debugfs.c and the other hyperv_debugfs.h.
I could put the code definitions in hv_debugfs.c and at the top
include the hyperv_debugfs.h file which would house the declarations
of these functions under the ifdef. Then like you alluded too use
an #else statement that would have the null implementations of the
above functions. Then put an #include "hyperv_debugfs.h" in the
hyperv_vmbus.h file. I figured instead of putting the code directly
into the vmbus_drv.c file it might be best to put them in a seperate
file like hv_debugfs.c. This way when we start adding more tests we
don't bloat the vmbus_drv.c file unnecessarily. The hv_debugfs.c
file would have the #ifdef CONFIG_HYPERV_TESTING at the top so if
its not enabled  those null implementations in "hyperv_debugfs.h"
woud kick in anywhere that included the hyperv_vmbus.h file which
is what we want.

what do you think?

> 
> >  	do {
> >  		void (*callback_fn)(void *);
> > 
> > diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> > index 362e70e9d145..edf14f596d8c 100644
> > --- a/drivers/hv/hyperv_vmbus.h
> > +++ b/drivers/hv/hyperv_vmbus.h
> > @@ -357,4 +357,24 @@ enum hvutil_device_state {
> >  	HVUTIL_DEVICE_DYING,     /* driver unload is in progress */
> >  };
> > 
> > +#ifdef CONFIG_HYPERV_TESTING
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> 
> Generally #include files should go at the top of the file, even if they
> are only needed conditionally.
>

I see , will change

> > +#define TESTING "hyperv"
> 
> I'm not seeing what this line is for, or how it is used.

I used it as the top level name for the dentry that
would appear in debugfs but now I realize its actually
not needed, so i'll remove this.

> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -926,6 +926,21 @@ struct vmbus_channel {
> >  	 * full outbound ring buffer.
> >  	 */
> >  	u64 out_full_first;
> > +
> > +#ifdef CONFIG_HYPERV_TESTING
> > +	/* enabling/disabling fuzz testing on the channel (default is false)*/
> > +	bool fuzz_testing_state;
> > +
> > +	/* Interrupt delay will delay the guest from emptying the ring buffer
> > +	 * for a specific amount of time. The delay is in microseconds and will
> > +	 * be between 1 to a maximum of 1000, its default is 0 (no delay).
> > +	 * The  Message delay will delay guest reading on a per message basis
> > +	 * in microseconds between 1 to 1000 with the default being 0
> > +	 * (no delay).
> > +	 */
> > +	u32 fuzz_testing_interrupt_delay;
> > +	u32 fuzz_testing_message_delay;
> > +#endif /* CONFIG_HYPERV_TESTING */
> 
> For fields in a data structure like this, you don't have much choice
> but to put the #ifdef directly inline.  However, for small fields like this
> and where the data structure isn't size sensitive, you could consider
> omitting the #ifdef and just always including the fields even when
> HYPERV_TESTING is not enabled.  I don't have a strong preference
> either way.
>

I'll take the ifdefs out since the fields aren't too big