netdev - Re: [PATCH 1/2][RFC] virtio_net: Enable setting MAC, promisc, and allmulti mode

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]

Date:	Fri, 09 Jan 2009 08:34:48 -0700
From:	Alex Williamson <alex.williamson@...com>
To:	Mark McLoughlin <markmc@...hat.com>
Cc:	Rusty Russell <rusty@...tcorp.com.au>, kvm <kvm@...r.kernel.org>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/2][RFC] virtio_net: Enable setting MAC, promisc, and
 allmulti mode

On Fri, 2009-01-09 at 11:34 +0000, Mark McLoughlin wrote:
> On Wed, 2009-01-07 at 11:06 -0700, Alex Williamson wrote:
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -23,6 +23,8 @@
> >  #define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
> >  
> >  #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
> > +#define VIRTIO_NET_S_PROMISC	2	/* Promiscuous mode */

Thanks for the rest of your comments and tips, I'll incorporate them.

> The current behaviour is PROMISC, right?
> 
> So, by not setting this flag you get PROMISC anyway?
> 
> Does that suggest we need a feature flag for this, or make zero reflect
> the current host behaviour ... i.e. change the bit to S_CHASTE ?
> 
> (Chaste is the antonym to promiscuous, apparently :-)

We could just have the "power on" state of virtio-net set the PROMISC
status bit, so it's up to the driver to clear it.  That's what I did for
the case of doing a load-vm from an older save state.  I think that will
give us the right behavior, but it's not clear to me if we need another
feature bit.

> > +#define VIRTIO_NET_S_ALLMULTI	4	/* All-multicast mode */
> 
> An issue here is that the LINK_UP bit is controlled by the host, but the
> PROMISC and ALLMULTI are controlled by the guest - i.e. they can race.
> 
> Suggest giving the top 8 bits to the guest and keeping the bottom 8 bits
> for the host.

Good thought, I was simply ignoring writes to the link bit.  AFAIK, it's
not uncommon that a status register on real hardware might have both RO
and RW bits next to each other.  I'm also toying with the idea that if
the mac table and vlan table gets moved to a virt-queue, maybe setting
and clearing these bits should be done via the vq too.  Thoughts?

> >  struct virtio_net_config
> >  {
> > @@ -30,7 +32,14 @@ struct virtio_net_config
> >  	__u8 mac[6];
> >  	/* Status supplied by host; see VIRTIO_NET_F_STATUS and VIRTIO_NET_S_*
> >  	 * bits above */
> > -	__u16 status;
> > +	union {
> > +		__u16 raw;
> > +		struct {
> > +			__u16 link:1;
> > +			__u16 promisc:1;
> > +			__u16 allmulti:1;
> > +		} bits;
> > +       } status;
> 
> Agree with Anthony on this, much rather we continued using the #defines.

Yeah, I do too now.  I've got a local copy working this way and it's no
messier.  Thanks,

Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html