| Message ID | 20171003173118.32645-1-robdclark@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] usb: kbd: Fix dangling pointers on probe fail | expand |
On Tue, Oct 3, 2017 at 6:31 PM, Rob Clark <robdclark@gmail.com> wrote: > If probe fails, we should unregister the stdio device, else we leave > dangling pointers to the parent 'struct usb_device'. > > Signed-off-by: Rob Clark <robdclark@gmail.com> Tested-by: Peter Robinson <pbrobinson@gmail.com> Tested on RPi3 and a couple of other devices. > --- > I finally got around to debugging why things explode so badly without > fixing usb_kbd vs. iomux[1] (which this patch applies on top of). > > [1] https://patchwork.ozlabs.org/patch/818881/ > > common/usb_kbd.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/common/usb_kbd.c b/common/usb_kbd.c > index 4c3ad95fca..82ad93f6ca 100644 > --- a/common/usb_kbd.c > +++ b/common/usb_kbd.c > @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev) > error = iomux_doenv(stdin, stdinname); > free(newstdin); > if (error) > - return error; > + goto unregister_stdio; > } else { > /* Check if this is the standard input device. */ > - if (strcmp(stdinname, DEVNAME)) > - return 1; > + if (strcmp(stdinname, DEVNAME)) { > + error = -1; > + goto unregister_stdio; > + } > > /* Reassign the console */ > - if (overwrite_console()) > - return 1; > + if (overwrite_console()) { > + error = -1; > + goto unregister_stdio; > + } > > error = console_assign(stdin, DEVNAME); > if (error) > - return error; > + goto unregister_stdio; > } > > return 0; > + > +unregister_stdio: > + /* > + * If probe fails, the device will be removed.. leaving dangling > + * pointers if the stdio device is not unregistered. If u-boot > + * is built without stdio_deregister(), just pretend to succeed > + * in order to avoid dangling pointers. > + */ > +#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER) > + stdio_deregister(DEVNAME, 1); > + return error; > +#else > + return 0; > +#endif > } > > #ifndef CONFIG_DM_USB > -- > 2.13.5 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
Hi Rob, On Wed, Oct 4, 2017 at 1:31 AM, Rob Clark <robdclark@gmail.com> wrote: > If probe fails, we should unregister the stdio device, else we leave > dangling pointers to the parent 'struct usb_device'. > > Signed-off-by: Rob Clark <robdclark@gmail.com> > --- > I finally got around to debugging why things explode so badly without > fixing usb_kbd vs. iomux[1] (which this patch applies on top of). > > [1] https://patchwork.ozlabs.org/patch/818881/ > > common/usb_kbd.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/common/usb_kbd.c b/common/usb_kbd.c > index 4c3ad95fca..82ad93f6ca 100644 > --- a/common/usb_kbd.c > +++ b/common/usb_kbd.c > @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev) > error = iomux_doenv(stdin, stdinname); > free(newstdin); > if (error) > - return error; > + goto unregister_stdio; > } else { > /* Check if this is the standard input device. */ > - if (strcmp(stdinname, DEVNAME)) > - return 1; > + if (strcmp(stdinname, DEVNAME)) { > + error = -1; Could we use some meaningful errno? > + goto unregister_stdio; > + } > > /* Reassign the console */ > - if (overwrite_console()) > - return 1; > + if (overwrite_console()) { > + error = -1; > + goto unregister_stdio; > + } > > error = console_assign(stdin, DEVNAME); > if (error) > - return error; > + goto unregister_stdio; > } > > return 0; > + > +unregister_stdio: > + /* > + * If probe fails, the device will be removed.. leaving dangling > + * pointers if the stdio device is not unregistered. If u-boot nits: U-Boot > + * is built without stdio_deregister(), just pretend to succeed > + * in order to avoid dangling pointers. > + */ > +#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER) > + stdio_deregister(DEVNAME, 1); > + return error; > +#else > + return 0; Don't understand why 'return 0' here if probe fails... > +#endif > } > > #ifndef CONFIG_DM_USB > -- Regards, Bin
On Thu, Oct 5, 2017 at 7:48 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Rob, > > On Wed, Oct 4, 2017 at 1:31 AM, Rob Clark <robdclark@gmail.com> wrote: >> If probe fails, we should unregister the stdio device, else we leave >> dangling pointers to the parent 'struct usb_device'. >> >> Signed-off-by: Rob Clark <robdclark@gmail.com> >> --- >> I finally got around to debugging why things explode so badly without >> fixing usb_kbd vs. iomux[1] (which this patch applies on top of). >> >> [1] https://patchwork.ozlabs.org/patch/818881/ >> >> common/usb_kbd.c | 30 ++++++++++++++++++++++++------ >> 1 file changed, 24 insertions(+), 6 deletions(-) >> >> diff --git a/common/usb_kbd.c b/common/usb_kbd.c >> index 4c3ad95fca..82ad93f6ca 100644 >> --- a/common/usb_kbd.c >> +++ b/common/usb_kbd.c >> @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev) >> error = iomux_doenv(stdin, stdinname); >> free(newstdin); >> if (error) >> - return error; >> + goto unregister_stdio; >> } else { >> /* Check if this is the standard input device. */ >> - if (strcmp(stdinname, DEVNAME)) >> - return 1; >> + if (strcmp(stdinname, DEVNAME)) { >> + error = -1; > > Could we use some meaningful errno? suggestions welcome.. I wasn't sure what to pick so I just went with <0 >> + goto unregister_stdio; >> + } >> >> /* Reassign the console */ >> - if (overwrite_console()) >> - return 1; >> + if (overwrite_console()) { >> + error = -1; >> + goto unregister_stdio; >> + } >> >> error = console_assign(stdin, DEVNAME); >> if (error) >> - return error; >> + goto unregister_stdio; >> } >> >> return 0; >> + >> +unregister_stdio: >> + /* >> + * If probe fails, the device will be removed.. leaving dangling >> + * pointers if the stdio device is not unregistered. If u-boot > > nits: U-Boot > >> + * is built without stdio_deregister(), just pretend to succeed >> + * in order to avoid dangling pointers. >> + */ >> +#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER) >> + stdio_deregister(DEVNAME, 1); >> + return error; >> +#else >> + return 0; > > Don't understand why 'return 0' here if probe fails... see above comment, basically we can't fail after we've registered the stdio device unless we can remove it. Suggestions welcome on better wording for the comment if it wasn't clear BR, -R >> +#endif >> } >> >> #ifndef CONFIG_DM_USB >> -- > > Regards, > Bin
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 4c3ad95fca..82ad93f6ca 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev) error = iomux_doenv(stdin, stdinname); free(newstdin); if (error) - return error; + goto unregister_stdio; } else { /* Check if this is the standard input device. */ - if (strcmp(stdinname, DEVNAME)) - return 1; + if (strcmp(stdinname, DEVNAME)) { + error = -1; + goto unregister_stdio; + } /* Reassign the console */ - if (overwrite_console()) - return 1; + if (overwrite_console()) { + error = -1; + goto unregister_stdio; + } error = console_assign(stdin, DEVNAME); if (error) - return error; + goto unregister_stdio; } return 0; + +unregister_stdio: + /* + * If probe fails, the device will be removed.. leaving dangling + * pointers if the stdio device is not unregistered. If u-boot + * is built without stdio_deregister(), just pretend to succeed + * in order to avoid dangling pointers. + */ +#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER) + stdio_deregister(DEVNAME, 1); + return error; +#else + return 0; +#endif } #ifndef CONFIG_DM_USB
If probe fails, we should unregister the stdio device, else we leave dangling pointers to the parent 'struct usb_device'. Signed-off-by: Rob Clark <robdclark@gmail.com> --- I finally got around to debugging why things explode so badly without fixing usb_kbd vs. iomux[1] (which this patch applies on top of). [1] https://patchwork.ozlabs.org/patch/818881/ common/usb_kbd.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)