From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754867AbdC1Ixq (ORCPT ); Tue, 28 Mar 2017 04:53:46 -0400 Received: from mx2.suse.de ([195.135.220.15]:39161 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754563AbdC1Ixp (ORCPT ); Tue, 28 Mar 2017 04:53:45 -0400 Message-ID: <1490688283.29898.2.camel@suse.com> Subject: Re: [PATCH v2 1/4] cdc-acm: reassemble fragmented notifications From: Oliver Neukum To: Tobias Herzog Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Date: Tue, 28 Mar 2017 10:04:43 +0200 In-Reply-To: <1490392212.2779.33.camel@gmx.de> References: <1479118868.21146.4.camel@suse.com> <1489863159-10972-1-git-send-email-t-herzog@gmx.de> <1489863159-10972-2-git-send-email-t-herzog@gmx.de> <1490022179.25734.5.camel@suse.com> <1490392212.2779.33.camel@gmx.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Am Freitag, den 24.03.2017, 22:50 +0100 schrieb Tobias Herzog: > Hi Oliver, > > thank you for your patience... :) I have a question to one of your > comments (see below). > > Best regards, > Tobias > > > > > Am Samstag, den 18.03.2017, 19:52 +0100 schrieb Tobias Herzog: > > > > > > > > > USB devices may have very limitited endpoint packet sizes, so that > > > notifications can not be transferred within one single usb packet. > > > Reassembling of multiple packages may be necessary. > > Hi, > > > > almost perfect. > > A few new issue. Comments inline. > > > > > > > > > > > > > > + struct usb_cdc_notification *dr = (struct > > > usb_cdc_notification *)buf; > > > + unsigned char *data = (unsigned char *)(dr + 1); > > This is border line incorrect. It depends on the compiler not > > adding padding. Please make it > > > > buf + sizeof(struct usb_cdc_notification) > > > > > > > > > > > - usb_mark_last_busy(acm->dev); > > > - > > > - data = (unsigned char *)(dr + 1); > > >   switch (dr->bNotificationType) { > > >   case USB_CDC_NOTIFY_NETWORK_CONNECTION: > > >   dev_dbg(&acm->control->dev, > > > @@ -363,8 +337,77 @@ static void acm_ctrl_irq(struct urb *urb) > > >   __func__, > > >   dr->bNotificationType, dr->wIndex, > > >   dr->wLength, data[0], data[1]); > > > + } > > > +} > > > + > > > +/* control interface reports status changes with "interrupt" > > > transfers */ > > > +static void acm_ctrl_irq(struct urb *urb) > > > +{ > > > + struct acm *acm = urb->context; > > > + struct usb_cdc_notification *dr = urb->transfer_buffer; > > > + unsigned int current_size = urb->actual_length; > > > + unsigned int expected_size, copy_size; > > > + int retval; > > > + int status = urb->status; > > > + > > > + switch (status) { > > > + case 0: > > > + /* success */ > > >   break; > > > + case -ECONNRESET: > > > + case -ENOENT: > > > + case -ESHUTDOWN: > > > + /* this urb is terminated, clean up */ > > > + acm->nb_index = 0; > > > + dev_dbg(&acm->control->dev, > > > + "%s - urb shutting down with status: > > > %d\n", > > > + __func__, status); > > > + return; > > > + default: > > > + dev_dbg(&acm->control->dev, > > > + "%s - nonzero urb status received: %d\n", > > > + __func__, status); > > > + goto exit; > > >   } > > > + > > > + usb_mark_last_busy(acm->dev); > > > + > > > + if (acm->nb_index) > > > + dr = (struct usb_cdc_notification *)acm- > > > > > > > > notification_buffer; > > > + > > > + /* size = notification-header + (optional) data */ > > > + expected_size = sizeof(struct usb_cdc_notification) + > > > + le16_to_cpu(dr->wLength); > > > + > > > + if (current_size < expected_size) { > > > + /* notification is transmitted fragmented, > > > reassemble */ > > > + if (acm->nb_size < expected_size) { > > > + if (acm->nb_size) { > > > + kfree(acm->notification_buffer); > > > + acm->nb_size = 0; > > > + } > > > + acm->notification_buffer = > > > + kmalloc(expected_size, > > > GFP_ATOMIC); > > Given how kmalloc works you'd better round this up to a power of two. > > > > > > > > > > > + if (!acm->notification_buffer) > > > + goto exit; > > This is most subtle. Please add a comment that this prevents a double > > free if we get a disconnect() > I'm unsure if I got this right: Are you talking about the fact, that > the use of 'kmalloc' will make 'acm->notification_buffer' valid again > (i.e. NULL or pointing to a correctly allocated block), after it was yes > "invalidated" by 'kfree' (in case the previously allocated buffer was > too small)? > The 'goto exit'-thing for me is just not to use the buffer if > allocation fails. Or am I missing anything here? You need to consider the hot unplug case. Regards Oliver