From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: Fix dumping ISOCHRONOUS IN transfers
To: tech@openbsd.org
Date: Thu, 19 Dec 2024 11:00:26 +0100

On 19/12/24(Thu) 02:27, Kirill A. Korinsky wrote:
> tech@,
> 
> tcpdump can be used to capture USB traffic, and it works well with one
> exception: ISOCHRONOUS IN transfers.
> 
> While OUT transfers work as expected, IN transfers only capture the first
> packet that can be read.
> 
> The root cause is that the same array is being used to store both the
> offsets in the data buffer and the resulting lengths. As a result, the dump
> records the length of the packets read from the device as offsets in the
> data buffer.

I don't see any offset being saved.  It is just the length of every
frame, no?

> When the device returns full packets, everything works correctly. However,
> when it returns partial packets, only the first packet is useful, and all
> subsequent packets are lost.

I'm not sure what you mean with "lost".  Are you saying that as soon as
there is a frame missed the offset calculation is incorrect?

Are you testing on ehci(4) or xhci(4)?  If it is xhci(4) is it because
`frlengths' is set to 0 in xhci_device_isoc_start()?

> Index: sys/dev/usb/usb.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/usb.c,v
> diff -u -p -r1.133 usb.c
> --- sys/dev/usb/usb.c	4 Sep 2024 07:54:52 -0000	1.133
> +++ sys/dev/usb/usb.c	19 Dec 2024 01:25:24 -0000
> @@ -952,7 +952,7 @@ usb_tap(struct usbd_bus *bus, struct usb
>  		struct usbpcap_iso_hdr_full	uih;
>  	} h;
>  	struct usbpcap_pkt_hdr *uph = &h.uch.uch_hdr;
> -	uint32_t nframes, offset;
> +	uint32_t nframes;
>  	unsigned int bpfdir;
>  	void *data = NULL;
>  	size_t flen;
> @@ -970,7 +970,6 @@ usb_tap(struct usbd_bus *bus, struct usb
>  		uph->uph_xfertype = USBPCAP_TRANSFER_CONTROL;
>  		break;
>  	case UE_ISOCHRONOUS:
> -		offset = 0;
>  		nframes = xfer->nframes;
>  #ifdef DIAGNOSTIC
>  		if (nframes > _USBPCAP_MAX_ISOFRAMES) {
> @@ -987,11 +986,10 @@ usb_tap(struct usbd_bus *bus, struct usb
>  		h.uih.uih_nframes = nframes;
>  		h.uih.uih_errors = 0; /* we don't have per-frame error */
>  		for (i = 0; i < nframes; i++) {
> -			h.uih.uih_frames[i].uip_offset = offset;
> +			h.uih.uih_frames[i].uip_offset = xfer->froffsets[i];
>  			h.uih.uih_frames[i].uip_length = xfer->frlengths[i];
>  			/* See above, we don't have per-frame error */
>  			h.uih.uih_frames[i].uip_status = 0;
> -			offset += xfer->frlengths[i];
>  		}
>  		break;
>  	case UE_BULK:
> @@ -1049,6 +1047,12 @@ usb_tap(struct usbd_bus *bus, struct usb
>  			if (xfer->rqflags & URQ_REQUEST)
>  				h.uch.uch_stage = USBPCAP_CONTROL_STAGE_STATUS;
>  		}
> +	}
> +
> +	/* ISOCHRONOUS IN from device may have gaps, use full buffer */
> +	if (bpfdir == BPF_DIRECTION_IN && uph->uph_dlen > 0 &&
> +	    uph->uph_xfertype == USBPCAP_TRANSFER_ISOCHRONOUS) {
> +		uph->uph_dlen = xfer->length;
>  	}
>  
>  	/* Dump bulk/intr/iso data, ctrl DATA or STATUS stage. */
> Index: sys/dev/usb/usbdi.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/usbdi.c,v
> diff -u -p -r1.111 usbdi.c
> --- sys/dev/usb/usbdi.c	23 May 2024 03:21:09 -0000	1.111
> +++ sys/dev/usb/usbdi.c	18 Dec 2024 22:35:39 -0000
> @@ -482,8 +482,10 @@ usbd_setup_isoc_xfer(struct usbd_xfer *x
>  	xfer->priv = priv;
>  	xfer->buffer = 0;
>  	xfer->length = 0;
> -	for (i = 0; i < nframes; i++)
> +	for (i = 0; i < nframes; i++) {
> +		xfer->froffsets[i] = xfer->length;
>  		xfer->length += frlengths[i];
> +	}
>  	xfer->actlen = 0;
>  	xfer->flags = flags;
>  	xfer->timeout = USBD_NO_TIMEOUT;
> Index: sys/dev/usb/usbdivar.h
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/usbdivar.h,v
> diff -u -p -r1.84 usbdivar.h
> --- sys/dev/usb/usbdivar.h	8 Oct 2024 19:42:31 -0000	1.84
> +++ sys/dev/usb/usbdivar.h	18 Dec 2024 22:35:42 -0000
> @@ -223,6 +223,12 @@ struct usbd_xfer {
>  	/* For isoc */
>  	u_int16_t		*frlengths;
>  	int			nframes;
> +/*
> + * OpenBSD specific, maximum number of frames per transfer used across
> + * all USB drivers.  This allows us to dump ISOC IN.
> + */
> +#define _USBD_MAX_ISOFRAMES	40
> +	u_int16_t		froffsets[_USBD_MAX_ISOFRAMES];
>  
>  	/* For memory allocation */
>  	struct usbd_device     *device;
> 
> -- 
> wbr, Kirill
>