netdev - Re: [PATCH v2 3/4] virtio_net: Add a MAC filter table

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: <200901301616.26914.rusty@rustcorp.com.au>
Date:	Fri, 30 Jan 2009 16:16:26 +1030
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Alex Williamson <alex.williamson@...com>
Cc:	markmc@...hat.com, netdev@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v2 3/4] virtio_net: Add a MAC filter table

On Friday 30 January 2009 09:35:18 Alex Williamson wrote:
>  	struct virtnet_info *vi = netdev_priv(dev);
>  	u8 promisc, allmulti;
> +	struct scatterlist sg[4];
> +	struct virtio_net_ctrl_hdr ctrl;
> +	virtio_net_ctrl_ack status;
> +	u8 *uc_buf = NULL, *mc_buf = NULL;
> +	unsigned int tmp;
>  
>  	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
>  		return;

It's kind of annoying that we have our virtnet_send_command helper and
yet we can't use it here.

Hmm, maybe we can kill two birds in one stone?  I was a bit nervous about the virtnet_send_command data arg because the pointer must not be vmalloc'ed memory.  If we make the arg to virtnet_send_command an sg[] in place of data & len, we force the caller to do the sg_set_buf (which means they *should* be aware of the limitation on what can be put in an sg), and we can make a copy (ideally we could chain it, but it's not possible to chain a const sg[], so let's BUG_ON if it's too big to fit on the stack).

Then this code can use it.

> -	promisc = ((dev->flags & IFF_PROMISC) != 0 || dev->uc_count > 0);
> -	allmulti = ((dev->flags & IFF_ALLMULTI) != 0 || dev->mc_count > 0);
> +	promisc = ((dev->flags & IFF_PROMISC) != 0);
> +	allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
> +
> +	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC)) {
> +		promisc |= (dev->uc_count > 0);
> +		allmulti |= (dev->mc_count > 0);
> +	}
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
>  				  VIRTIO_NET_CTRL_RX_PROMISC,
> @@ -682,6 +692,79 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>  				  &allmulti, sizeof(allmulti)))
>  		printk(KERN_WARNING "%s: Failed to %sable allmulti mode.\n",
>  		       dev->name, allmulti ? "en" : "dis");
> +
> +	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC))
> +		return;

I think we can now fold this feature into VIRTIO_NET_F_CTRL_RX and assert that you must support mac filtering.  Because it's now trivial to support in the host (use promisc!), and it simplifies the implementation.

> +	if (dev->uc_count) {
...
> +		sg_set_buf(&sg[1], uc_buf, dev->uc_count * ETH_ALEN);
...
> +	if (dev->mc_count) {
...
> +		sg_set_buf(&sg[2], mc_buf, dev->mc_count * ETH_ALEN);

We made the decision a while ago not to rely on scatter gather boundaries to define the API.  So you'll actually need a structure to contain the counts unfortunately (technically you only need one count, since the other can be intuited, but that seems silly, and this way you could theoretically add more fields without problems with future feature bits).

It also means you can do a single kmalloc, and skip the if here, if you wanted.

Cheers,
Rusty.
--
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