> On 2 Jan 2016, at 7:55 AM, Stefan Fritsch <s...@sfritsch.de> wrote:
>
> Hi,
>
> by default, the ether_input() checks the destination MAC address of
> incoming unicast packets only if the interface is in promiscous mode. If
> not, it is assumed that the NIC filters unicast packets reliably.
> Unfortunately, for virtio-net this is not the case. There, unicast
> filtering is only best effort, and (depending on configuration) if the
> bridge on the VM host does unicast flodding, unicast packets that are not
> for the VM guest may still be delivered to the VM guest. This is a rather
> annoying problem because it can cause pf to send RST packets to foreign
> connections. (Kudos to mpf@ for debugging this).
>
> There are two possible approaches to fix this problem. Either make the
> vio(4) driver filter out unicast packets that are not for the local MAC,
> which would involve duplicating quite a bit of code from ether_input() in
> vio(4). Or, and I would prefer this, allow the driver to tell
> ether_input() that it needs to check the MAC always, and not only if the
> interface is in promiscous mode.
>
> This could be done with a new flag. There seem to be three possible places
> where this flag could be put:
>
> * ifnet.if_flags
> This is a short and there is no free bit. But the IFF_NOTRAILERS bit has
> become unused recently and could be recycled.
>
> * ifnet.if_xflags
> An int, lots of free bits. But comment says 'extra softnet flags'
>
> * if_data.ifi_capabilities
> An u_int32_t, lots of free bits.
>
>
> In the diff below, I went with the first choice because the new
> IFF_NOMACFILTER
> is somewhat similar to IFF_SIMPLEX and because the the check can then be
> nicely folded into the existing check for IFF_PROMISC.
>
> I would welcome any comments, suggestions for a better flag name, OKs, ...
should we just do it unconditionally? is there a downside to that?
dlg
>
> Cheers,
> Stefan
>
>
>
> diff --git sys/dev/pci/if_vio.c sys/dev/pci/if_vio.c
> index 4cd80d5..22fd7cf 100644
> --- sys/dev/pci/if_vio.c
> +++ sys/dev/pci/if_vio.c
> @@ -582,21 +582,21 @@ vio_attach(struct device *parent, struct device *self,
> void *aux)
> virtio_start_vq_intr(vsc, &sc->sc_vq[VQCTL]);
> vsc->sc_nvqs = 3;
> }
> }
>
> if (vio_alloc_mem(sc) < 0)
> goto err;
>
> strlcpy(ifp->if_xname, self->dv_xname, IFNAMSIZ);
> ifp->if_softc = sc;
> - ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
> + ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST |
> IFF_NOMACFILTER;
> ifp->if_start = vio_start;
> ifp->if_ioctl = vio_ioctl;
> ifp->if_capabilities = IFCAP_VLAN_MTU;
> if (features & VIRTIO_NET_F_CSUM)
> ifp->if_capabilities |= IFCAP_CSUM_TCPv4|IFCAP_CSUM_UDPv4;
> IFQ_SET_MAXLEN(&ifp->if_snd, vsc->sc_vqs[1].vq_num - 1);
> IFQ_SET_READY(&ifp->if_snd);
> ifmedia_init(&sc->sc_media, 0, vio_media_change, vio_media_status);
> ifmedia_add(&sc->sc_media, IFM_ETHER | IFM_AUTO, 0, NULL);
> ifmedia_set(&sc->sc_media, IFM_ETHER | IFM_AUTO);
> diff --git sys/net/if.h sys/net/if.h
> index 8d7e390..91c6d18 100644
> --- sys/net/if.h
> +++ sys/net/if.h
> @@ -182,36 +182,37 @@ struct if_status_description {
> /*
> * Length of interface description, including terminating '\0'.
> */
> #define IFDESCRSIZE 64
>
> #define IFF_UP 0x1 /* interface is up */
> #define IFF_BROADCAST 0x2 /* broadcast address valid */
> #define IFF_DEBUG 0x4 /* turn on debugging */
> #define IFF_LOOPBACK 0x8 /* is a loopback net */
> #define IFF_POINTOPOINT 0x10 /* interface is point-to-point
> link */
> -#define IFF_NOTRAILERS 0x20 /* avoid use of trailers */
> +#define IFF_NOMACFILTER 0x20 /* Does not reliably filter
> unicast MACs */
> #define IFF_RUNNING 0x40 /* resources allocated */
> #define IFF_NOARP 0x80 /* no address resolution
> protocol */
> #define IFF_PROMISC 0x100 /* receive all packets */
> #define IFF_ALLMULTI 0x200 /* receive all multicast
> packets */
> #define IFF_OACTIVE 0x400 /* transmission in progress */
> #define IFF_SIMPLEX 0x800 /* can't hear own transmissions
> */
> #define IFF_LINK0 0x1000 /* per link layer defined bit */
> #define IFF_LINK1 0x2000 /* per link layer defined bit */
> #define IFF_LINK2 0x4000 /* per link layer defined bit */
> #define IFF_MULTICAST 0x8000 /* supports multicast */
>
> +
> /* flags set internally only: */
> #define IFF_CANTCHANGE \
> (IFF_BROADCAST|IFF_POINTOPOINT|IFF_RUNNING|IFF_OACTIVE|\
> - IFF_SIMPLEX|IFF_MULTICAST|IFF_ALLMULTI)
> + IFF_SIMPLEX|IFF_MULTICAST|IFF_ALLMULTI|IFF_NOMACFILTER)
>
> #define IFXF_MPSAFE 0x1 /* if_start is mpsafe */
> #define IFXF_INET6_NOPRIVACY 0x4 /* don't autoconf
> privacy */
> #define IFXF_MPLS 0x8 /* supports MPLS */
> #define IFXF_WOL 0x10 /* wake on lan enabled
> */
> #define IFXF_AUTOCONF6 0x20 /* v6 autoconf enabled
> */
>
> #define IFXF_CANTCHANGE \
> (IFXF_MPSAFE)
>
> diff --git sys/net/if_ethersubr.c sys/net/if_ethersubr.c
> index 2a44161..296e6f6 100644
> --- sys/net/if_ethersubr.c
> +++ sys/net/if_ethersubr.c
> @@ -347,21 +347,21 @@ ether_input(struct ifnet *ifp, struct mbuf *m, void
> *cookie)
> if (m->m_flags & (M_FILDROP | M_VLANTAG)) {
> m_freem(m);
> return (1);
> }
>
> /*
> * If packet is unicast and we're in promiscuous mode, make sure it
> * is for us. Drop otherwise.
> */
> if ((m->m_flags & (M_BCAST|M_MCAST)) == 0 &&
> - (ifp->if_flags & IFF_PROMISC)) {
> + (ifp->if_flags & (IFF_PROMISC|IFF_NOMACFILTER))) {
> if (memcmp(ac->ac_enaddr, eh->ether_dhost, ETHER_ADDR_LEN)) {
> m_freem(m);
> return (1);
> }
> }
>
> etype = ntohs(eh->ether_type);
>
> decapsulate:
> switch (etype) {
>
