On Sun, 3 Jan 2016, Theo de Raadt wrote:
> >On Friday 01 January 2016 16:25:59, Theo de Raadt wrote:
> >> dlg writes:
> >> > should we just do it unconditionally? is there a downside to that?
> >
> >It may decrease performance a tiny bit. Since such bits tend to add
> >up, I would be hesitant to enable the check for drivers that don't
> >need it. OTOH, freebsd and linux seem to always do the check.
>
> How does it decrease performance?
I am talking about this diff in ether_input():
diff --git sys/net/if_ethersubr.c sys/net/if_ethersubr.c
--- sys/net/if_ethersubr.c
+++ sys/net/if_ethersubr.c
@@ -353,8 +353,7 @@ ether_input(struct ifnet *ifp, struct mbuf *m, void *cookie)
* 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)) {
+ if ((m->m_flags & (M_BCAST|M_MCAST)) == 0) {
if (memcmp(ac->ac_enaddr, eh->ether_dhost, ETHER_ADDR_LEN)) {
m_freem(m);
return (1);
For other drivers than vio this means an additional 6 byte memcmp where
one operand is already in the cache, and the other operand may or may not
be in the cache. 'tiny performance impact' seems about right.
>
> I tried to read the rest of what you wrote. It makes no sense. Since
> these mbufs are coming off a ring, aren't they already linear; aren't
> these mbufs already normalzed ie. "pulled up". The performance cost
> seems to be one condition in a mbuf macro which decides to do no work.
>
> Maybe you should show the diff which does the check in the driver
> itself, then then it will be easier to see the performance cost. Right
> now I can't perceive the problem.
It's not very expensive. But it's more code duplication than I like:
diff --git sys/dev/pci/if_vio.c sys/dev/pci/if_vio.c
--- sys/dev/pci/if_vio.c
+++ sys/dev/pci/if_vio.c
@@ -1005,6 +1005,9 @@ vio_rxeof(struct vio_softc *sc)
int r = 0;
int slot, len, bufs_left;
struct virtio_net_hdr *hdr;
+ int promisc = (ifp->if_flags & IFF_PROMISC);
+ u_int8_t *enaddr = sc->sc_ac.ac_enaddr;
+ struct ether_header *eh;
while (virtio_dequeue(vsc, vq, &slot, &len) == 0) {
r = 1;
@@ -1035,10 +1038,33 @@ vio_rxeof(struct vio_softc *sc)
bufs_left--;
}
- if (bufs_left == 0) {
- ml_enqueue(&ml, m0);
- m0 = NULL;
+ if (bufs_left > 0)
+ continue;
+
+ /*
+ * Unfortunately, mac filtering is only best effort
+ * in virtio-net. Unwanted packets may still arrive.
+ * If we are not promiscous, we have to check if the
+ * packet is actually for us.
+ */
+ if (!promisc) {
+ m0 = m_pullup(m0, ETHER_HDR_LEN);
+ /*
+ * no need to check m0 == NULL, we know m0 has enough
+ * space
+ */
+
+ eh = mtod(m0, struct ether_header *);
+ if (!ETHER_IS_MULTICAST(eh->ether_dhost) &&
+ memcmp(enaddr, eh->ether_dhost, ETHER_ADDR_LEN) !=
0) {
+ m_freem(m0);
+ m0 = NULL;
+ continue;
+ }
}
+
+ ml_enqueue(&ml, m0);
+ m0 = NULL;
}
if (m0 != NULL) {
DBGPRINT("expected %d buffers, got %d", (int)hdr->num_buffers,
