On Tue, Jan 22, 2019 at 10:22:39AM -0200, Martin Pieuchot wrote: > On 31/10/18(Wed) 02:55, Artturi Alm wrote: > > Hi, > > > > spam in the subject does make ttyC0 unusable in practice with urndis(4). > > I don't like repeating myself, so all i provide is the link where this > > behaviour requiring while (len > 1) is explained: > > https://docs.microsoft.com/en-us/windows-hardware/drivers/network/usb-short-packets > > Could you explain in words why we need this change? What does it solve?
If the incoming packet size is (length % 64(=wMaxPacketSize) == 0), Linux does lenght++ to allow us to force short reads[0], they call this behaviour as "strict CDC-Ether conformance", our cdce(4) already does the same[1]. And as-is without s/return/break/, valid packet(s) would be lost before the 'zlp' at the end of it, even if the loop condition was fixed alone. does that make sense? -Artturi [0]: https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/u_ether.c#L578 [1]: https://github.com/openbsd/src/blame/master/sys/dev/usb/if_cdce.c#L745 > > > > > I'm running the minimal diff below on amd64 running 6.4. > > > > -Artturi > > > > > > diff --git a/sys/dev/usb/if_urndis.c b/sys/dev/usb/if_urndis.c > > index f6b3c7bad9d..c5b4bc4ed3c 100644 > > --- a/sys/dev/usb/if_urndis.c > > +++ b/sys/dev/usb/if_urndis.c > > @@ -826,7 +826,7 @@ urndis_decap(struct urndis_softc *sc, struct > > urndis_chain *c, u_int32_t len) > > ifp = GET_IFP(sc); > > offset = 0; > > > > - while (len > 0) { > > + while (len > 1) { > > msg = (struct rndis_packet_msg *)((char*)c->sc_buf + offset); > > m = c->sc_mbuf; > > > > @@ -839,7 +839,7 @@ urndis_decap(struct urndis_softc *sc, struct > > urndis_chain *c, u_int32_t len) > > DEVNAME(sc), > > len, > > sizeof(*msg)); > > - return; > > + break; > > } > > > > DPRINTF(("%s: urndis_decap len %u data(off:%u len:%u) " > > @@ -859,14 +859,14 @@ urndis_decap(struct urndis_softc *sc, struct > > urndis_chain *c, u_int32_t len) > > DEVNAME(sc), > > letoh32(msg->rm_type), > > REMOTE_NDIS_PACKET_MSG); > > - return; > > + break; > > } > > if (letoh32(msg->rm_len) < sizeof(*msg)) { > > printf("%s: urndis_decap invalid msg len %u < %zu\n", > > DEVNAME(sc), > > letoh32(msg->rm_len), > > sizeof(*msg)); > > - return; > > + break; > > } > > if (letoh32(msg->rm_len) > len) { > > printf("%s: urndis_decap invalid msg len %u > buffer " > > @@ -874,7 +874,7 @@ urndis_decap(struct urndis_softc *sc, struct > > urndis_chain *c, u_int32_t len) > > DEVNAME(sc), > > letoh32(msg->rm_len), > > len); > > - return; > > + break; > > } > > > > if (letoh32(msg->rm_dataoffset) + > > @@ -889,7 +889,7 @@ urndis_decap(struct urndis_softc *sc, struct > > urndis_chain *c, u_int32_t len) > > letoh32(msg->rm_dataoffset) + > > letoh32(msg->rm_datalen) + RNDIS_HEADER_OFFSET, > > letoh32(msg->rm_len)); > > - return; > > + break; > > } > > > > if (letoh32(msg->rm_datalen) < sizeof(struct ether_header)) { > > @@ -899,7 +899,7 @@ urndis_decap(struct urndis_softc *sc, struct > > urndis_chain *c, u_int32_t len) > > DEVNAME(sc), > > letoh32(msg->rm_datalen), > > sizeof(struct ether_header))); > > - return; > > + break; > > } > > > > memcpy(mtod(m, char*), > > @@ -916,6 +916,8 @@ urndis_decap(struct urndis_softc *sc, struct > > urndis_chain *c, u_int32_t len) > > offset += letoh32(msg->rm_len); > > len -= letoh32(msg->rm_len); > > } > > + if (ml_empty(&ml)) > > + return; > > > > s = splnet(); > > if_input(ifp, &ml); > >
