Re: kern/50810: Kernel page fault trap in ugenclose()

[Nick Hudson <skrll%netbsd.org@localhost>]

The following reply was made to PR kern/50810; it has been noted by GNATS.

From: Nick Hudson <skrll%netbsd.org@localhost>
To: Andreas Gustafsson <gson%gson.org@localhost>
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: kern/50810: Kernel page fault trap in ugenclose()
Date: Mon, 22 Feb 2016 08:11:58 +0000

 On 02/22/16 07:55, Andreas Gustafsson wrote:
 > Nick Hudson wrote:
 >> I got rid of these pointless conditionals
 > I'm just wondering if whoever put them in in the first place intended
 > to check for a valid endpoint, in which case some of them might need to
 > be replaced by working checks for a valid endpoint rather than just
 > removed.
 >
 >>>    >  > What is the canonical way
 >>>    >  > of distinguishing a valid endpoint from an invalid one?
 >>>    >
 >>>    >  I guess edesc and/or iface get populated with something after
 >>>    >  ugen_set_interface/ugen_set_config
 >>>    
 >>>    I'm sorry, but I fail to see the connection between my question and
 >>>    your answer.
 >> I was really answering your second question and not the first. Both
 >> edesc and iface will
 >> be non-null if the endpoint is valid.
 > That still doesn't tell me which one the code should be checking.
 > I guess the answer is that there is no canonical way, and that
 > each function is just checking whatever field it is going to
 > dereference (if it is checking at all).
 
 Huh? check either or both...
 
 >
 >> Does this diff help?
 > Building with UGENDEBUG enabled, I got:
 >
 >    /ssd/current/src/sys/dev/usb/ugen.c:274:1: error: no previous prototype for 'ugen_clear_endpoints' [-Werror=missing-prototypes]
 >     ugen_clear_endpoints(struct ugen_softc *sc)
 >     ^
 >    cc1: all warnings being treated as errors
 >    *** [ugen.o] Error code 1
 >
 > After I worked around that, the patch did prevent the crash, and not
 > only that, the scan even succeeded, which it didn't with my own
 > initial patch.  Thank you!
 >
 >> I'm not sure of retaining selinfo and the cv is correct for
 >> ugen_set_interface.
 > I'm not sure, either.
 
 Can you work out where in sane it's doing the 
 ioctl(USB_SET_ALTINTERFACE) and
 why it's choosing to do it before closing the endpoints of the existing 
 interface? #
 I'm curious what behaviour it's expecting.
 
 Thanks,
 Nick