Re: [PATCH v2 1/4] cdc-acm: reassemble fragmented notifications

[Oliver Neukum <oneukum@suse.com>]

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