* [RFC PATCHv2 0/1] usb: f_rndis: Avoid to use ERROR macro if cdev can be null @ 2013-03-19 11:11 oskar.andero 2013-03-19 11:11 ` [RFC PATCHv2 1/1] " oskar.andero 0 siblings, 1 reply; 4+ messages in thread From: oskar.andero @ 2013-03-19 11:11 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Greg Kroah-Hartman, Felipe Balbi, Radovan.Lekanovic, Oskar Andero Hi, This is patch version 2. Besides review I hope to get some feed-back on what the preferred solution is. Background: When going through our patches to be mainlined I stumbled on this one which we have fixed in many different ways internally. The problem is a NULL pointer dereference that can be triggered by disconnecting the USB cable at a specific time. Before submitting the final patch I would like to hear which solution you'd prefer. As I see it there are four different ways to fix the problem: 1) Remove the ERROR() call completely. 2) Add an if-statement on cdev in rndis_response_complete() and use pr_err() or ERROR(). 3) Globally update the ERROR() macro to handle the case where cdev is null. 4) Use the attached patch (RFC PATCHv2 1/1) where ERROR() is simply replaced with pr_err(). Thanks! -Oskar Truls Bengtsson (1): usb: f_rndis: Avoid to use ERROR macro if cdev can be null drivers/usb/gadget/f_rndis.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) -- 1.7.8.6 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [RFC PATCHv2 1/1] usb: f_rndis: Avoid to use ERROR macro if cdev can be null 2013-03-19 11:11 [RFC PATCHv2 0/1] usb: f_rndis: Avoid to use ERROR macro if cdev can be null oskar.andero @ 2013-03-19 11:11 ` oskar.andero 2013-03-19 13:22 ` Michal Nazarewicz 0 siblings, 1 reply; 4+ messages in thread From: oskar.andero @ 2013-03-19 11:11 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Greg Kroah-Hartman, Felipe Balbi, Radovan.Lekanovic, Truls Bengtsson, Oskar Andero From: Truls Bengtsson <truls.bengtsson@sonymobile.com> The udc_irq service runs the isr_tr_complete_handler which in turn "nukes" the endpoints, including a call to rndis_response_complete, if appropriate. If the rndis_msg_parser fails here, an error will be printed using a dev_err call (through the ERROR() macro). However, if the usb cable was just disconnected the device (cdev) might not be available and will be null. Since the dev_err macro will dereference the cdev pointer we get a null pointer exception. Reviewed-by: Radovan Lekanovic <radovan.lekanovic@sonymobile.com> Signed-off-by: Truls Bengtsson <truls.bengtsson@sonymobile.com> Signed-off-by: Oskar Andero <oskar.andero@sonymobile.com> --- drivers/usb/gadget/f_rndis.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/drivers/usb/gadget/f_rndis.c b/drivers/usb/gadget/f_rndis.c index 71beeb8..cc9c49c 100644 --- a/drivers/usb/gadget/f_rndis.c +++ b/drivers/usb/gadget/f_rndis.c @@ -447,14 +447,13 @@ static void rndis_response_complete(struct usb_ep *ep, struct usb_request *req) static void rndis_command_complete(struct usb_ep *ep, struct usb_request *req) { struct f_rndis *rndis = req->context; - struct usb_composite_dev *cdev = rndis->port.func.config->cdev; int status; /* received RNDIS command from USB_CDC_SEND_ENCAPSULATED_COMMAND */ // spin_lock(&dev->lock); status = rndis_msg_parser(rndis->config, (u8 *) req->buf); if (status < 0) - ERROR(cdev, "RNDIS command error %d, %d/%d\n", + pr_err("RNDIS command error %d, %d/%d\n", status, req->actual, req->length); // spin_unlock(&dev->lock); } -- 1.7.8.6 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCHv2 1/1] usb: f_rndis: Avoid to use ERROR macro if cdev can be null 2013-03-19 11:11 ` [RFC PATCHv2 1/1] " oskar.andero @ 2013-03-19 13:22 ` Michal Nazarewicz 2013-03-20 12:51 ` Felipe Balbi 0 siblings, 1 reply; 4+ messages in thread From: Michal Nazarewicz @ 2013-03-19 13:22 UTC (permalink / raw) To: oskar.andero, linux-kernel, linux-usb Cc: Greg Kroah-Hartman, Felipe Balbi, Radovan.Lekanovic, Truls Bengtsson, Oskar Andero [-- Attachment #1: Type: text/plain, Size: 2149 bytes --] On Tue, Mar 19 2013, oskar.andero@sonymobile.com wrote: > The udc_irq service runs the isr_tr_complete_handler which in turn > "nukes" the endpoints, including a call to rndis_response_complete, > if appropriate. If the rndis_msg_parser fails here, an error will > be printed using a dev_err call (through the ERROR() macro). > > However, if the usb cable was just disconnected the device (cdev) > might not be available and will be null. Since the dev_err macro will > dereference the cdev pointer we get a null pointer exception. > > Reviewed-by: Radovan Lekanovic <radovan.lekanovic@sonymobile.com> > Signed-off-by: Truls Bengtsson <truls.bengtsson@sonymobile.com> > Signed-off-by: Oskar Andero <oskar.andero@sonymobile.com> Acked-by: Michal Nazarewicz <mina86@mina86.com> I think this is the best solution. Adding if statements around it would just add noise. > --- > drivers/usb/gadget/f_rndis.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/f_rndis.c b/drivers/usb/gadget/f_rndis.c > index 71beeb8..cc9c49c 100644 > --- a/drivers/usb/gadget/f_rndis.c > +++ b/drivers/usb/gadget/f_rndis.c > @@ -447,14 +447,13 @@ static void rndis_response_complete(struct usb_ep *ep, struct usb_request *req) > static void rndis_command_complete(struct usb_ep *ep, struct usb_request *req) > { > struct f_rndis *rndis = req->context; > - struct usb_composite_dev *cdev = rndis->port.func.config->cdev; > int status; > > /* received RNDIS command from USB_CDC_SEND_ENCAPSULATED_COMMAND */ > // spin_lock(&dev->lock); > status = rndis_msg_parser(rndis->config, (u8 *) req->buf); > if (status < 0) > - ERROR(cdev, "RNDIS command error %d, %d/%d\n", > + pr_err("RNDIS command error %d, %d/%d\n", > status, req->actual, req->length); > // spin_unlock(&dev->lock); > } -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo-- [-- Attachment #2.1: Type: text/plain, Size: 0 bytes --] [-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCHv2 1/1] usb: f_rndis: Avoid to use ERROR macro if cdev can be null 2013-03-19 13:22 ` Michal Nazarewicz @ 2013-03-20 12:51 ` Felipe Balbi 0 siblings, 0 replies; 4+ messages in thread From: Felipe Balbi @ 2013-03-20 12:51 UTC (permalink / raw) To: Michal Nazarewicz Cc: oskar.andero, linux-kernel, linux-usb, Greg Kroah-Hartman, Felipe Balbi, Radovan.Lekanovic, Truls Bengtsson [-- Attachment #1: Type: text/plain, Size: 1090 bytes --] On Tue, Mar 19, 2013 at 02:22:56PM +0100, Michal Nazarewicz wrote: > On Tue, Mar 19 2013, oskar.andero@sonymobile.com wrote: > > The udc_irq service runs the isr_tr_complete_handler which in turn > > "nukes" the endpoints, including a call to rndis_response_complete, > > if appropriate. If the rndis_msg_parser fails here, an error will > > be printed using a dev_err call (through the ERROR() macro). > > > > However, if the usb cable was just disconnected the device (cdev) > > might not be available and will be null. Since the dev_err macro will > > dereference the cdev pointer we get a null pointer exception. > > > > Reviewed-by: Radovan Lekanovic <radovan.lekanovic@sonymobile.com> > > Signed-off-by: Truls Bengtsson <truls.bengtsson@sonymobile.com> > > Signed-off-by: Oskar Andero <oskar.andero@sonymobile.com> > > Acked-by: Michal Nazarewicz <mina86@mina86.com> > > I think this is the best solution. Adding if statements around it would > just add noise. alright, please re-send without RFC tag and with Michal's acked-by so I can apply. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-03-20 12:51 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-03-19 11:11 [RFC PATCHv2 0/1] usb: f_rndis: Avoid to use ERROR macro if cdev can be null oskar.andero 2013-03-19 11:11 ` [RFC PATCHv2 1/1] " oskar.andero 2013-03-19 13:22 ` Michal Nazarewicz 2013-03-20 12:51 ` Felipe Balbi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.