On Tue, Dec 03, 2013 at 01:07:17AM +0200, Paul Irofti wrote:
> On Tue, Dec 03, 2013 at 12:47:30AM +0200, Paul Irofti wrote:
> > Could people doing actual networking with urndis(4) please test the
> > following diff and report if any regressions are encountered?
> >
> > I plan on commiting it by the end of the week if I don't hear anything
> > bad in regards to the patch.
> >
>
> To be more specific, this diff fixes a panic on detach when connecting
> my phone to an OpenBSD machine.
Clarifying further, this is due to failure to setup a pipe in
urndis_ctrl_init() which results in a TIMEOUT on receive with
side-effects later on during detach due to assumptions regarding
the existence of interface hooks during dohooks on if_detach tear-down.
This results on dereferencing a NULL function pointer which triggers a
panic.
>
> The ctrl methods on attach only use control messages and don't need any
> pipe besides the default pipe which is always present.
>
> Thus the urndis_init() and, in turn, urndis_stop() funcalls are not
> needed during attach.
>
> Besides from that they're also poorly written as they can fail and that
> is not checked nor reported. This driver is in really poor shape.
>
> This diff just brings this awful driver closer to what other usb
> ethernet drivers are doing.
>
>
> Others diffs will probably follow in order to clean-up the current mess.
>
>
> > Index: if_urndis.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/if_urndis.c,v
> > retrieving revision 1.44
> > diff -u -p -r1.44 if_urndis.c
> > --- if_urndis.c 21 Nov 2013 14:08:05 -0000 1.44
> > +++ if_urndis.c 28 Nov 2013 10:22:42 -0000
> > @@ -1363,6 +1363,7 @@ urndis_attach(struct device *parent, str
> > sc = (void *)self;
> > uaa = aux;
> >
> > + sc->sc_attached = 0;
> > sc->sc_udev = uaa->device;
> > id = usbd_get_interface_descriptor(uaa->iface);
> > sc->sc_ifaceno_ctl = id->bInterfaceNumber;
> > @@ -1447,14 +1448,11 @@ urndis_attach(struct device *parent, str
> >
> > IFQ_SET_READY(&ifp->if_snd);
> >
> > - urndis_init(sc);
> > -
> > s = splnet();
> >
> > if (urndis_ctrl_query(sc, OID_802_3_PERMANENT_ADDRESS, NULL, 0,
> > &buf, &bufsz) != RNDIS_STATUS_SUCCESS) {
> > printf(": unable to get hardware address\n");
> > - urndis_stop(sc);
> > splx(s);
> > return;
> > }
> > @@ -1466,7 +1464,6 @@ urndis_attach(struct device *parent, str
> > } else {
> > printf(", invalid address\n");
> > free(buf, M_TEMP);
> > - urndis_stop(sc);
> > splx(s);
> > return;
> > }
> > @@ -1478,7 +1475,6 @@ urndis_attach(struct device *parent, str
> > if (urndis_ctrl_set(sc, OID_GEN_CURRENT_PACKET_FILTER, &filter,
> > sizeof(filter)) != RNDIS_STATUS_SUCCESS) {
> > printf("%s: unable to set data filters\n", DEVNAME(sc));
> > - urndis_stop(sc);
> > splx(s);
> > return;
> > }
> >
>
