netdev - Re: [PATCH v2 2/4] virtio_net: Add a set_rx

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, 30 Jan 2009 16:00:57 +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 2/4] virtio_net: Add a set_rx_mode interface

On Friday 30 January 2009 09:35:12 Alex Williamson wrote: 
> +static void virtnet_set_rx_mode(struct net_device *dev)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	u8 promisc, allmulti;
> +
> +	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
> +		return;

Hmm, this check duplicates the one in virtnet_send_command; after
all your patches, is it ever called with !VIRTIO_NET_F_CTRL_RX?  Maybe it should be a BUG_ON in there?

> +	promisc = ((dev->flags & IFF_PROMISC) != 0 || dev->uc_count > 0);
> +	allmulti = ((dev->flags & IFF_ALLMULTI) != 0 || dev->mc_count > 0);
> +
> +	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> +				  VIRTIO_NET_CTRL_RX_PROMISC,
> +				  &promisc, sizeof(promisc)))
> +		printk(KERN_WARNING "%s: Failed to %sable promisc mode.\n",
> +		       dev->name, promisc ? "en" : "dis");
> +
> +	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> +				  VIRTIO_NET_CTRL_RX_ALLMULTI,
> +				  &allmulti, sizeof(allmulti)))
> +		printk(KERN_WARNING "%s: Failed to %sable allmulti mode.\n",
> +		       dev->name, allmulti ? "en" : "dis");
> +}

Hmm, we can't do anything with this error.  I'd be very tempted to define the API to say "this can't fail".  Leave this code in as a sanity check, but have a comment to that effect?

> +#define VIRTIO_NET_CTRL_RX    0
> + #define VIRTIO_NET_CTRL_RX_PROMISC      0
> + #define VIRTIO_NET_CTRL_RX_ALLMULTI     1

Comment above/beside these two perhaps?
/* Supported if VIRTIO_NET_F_CTRL_RX */

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