ipheth: add ncm support for rx

[aokblast]

Summary

Iphone tethering enable ncm by default.

I don't know if iOS should take the responsibility or usbmuxd should. For short, we need ncm in ipheth driver to connected to the network.
Taking Linux code as the reference, iPhone currently implement ncm in rx side and has no CRC implemented.
Also, iOS has fixed 16 dp items in each usb packet regardless of the usb configuration.
Tested on my iOS 18.3.2

[aokblast]

@hrs maybe you would like to test it？：）

[aokblast]

Fix indent and header reorder

[aokblast]

Still some bug on the usbmuxd so that we have to choose the mode by environment variable like this:

sudo USBMUXD_DEFAULT_DEVICE_MODE=4 usbmuxd --enable-exit --foreground --user root --verbose

Also, with the USBMUXD_DEFAULT_DEVICE_MODE, we don't have to use usbconfig to change the mode.

[aokblast]

Handle ack and add missing special packets

[aokblast]

Remove useless .h include

[aokblast]

Currently, we don't need any mode change from usbmuxd or any special environment variable. The only thing we have to do is:

1. load if_ipheth.ko
2. plug your iphone
3. all things will be done through devd.

[aokblast]

Elaborate comit message

[lwhsu]

I have tested this and along with D51200, iPhone tethering works again now.

[emaste]

Please expand the NCM acronym somewhere (in a comment if appropriate, or at least in the commit message).

[lwhsu]

Note that currently we also need usbmuxd updated with this patch: https://people.freebsd.org/~jhale/patches/0001-imobiledevice-Update-suite-to-lastest-versions.patch

[markj]

Just a few style comments.

This looks good to me, but as Ed mentioned, the commit message should explain what NCM means.

sys/dev/usb/net/if_ipheth.c

151

567

Is this an unrelated change?

587

sys/dev/usb/net/if_iphethvar.h

71

[lwhsu]

sys/dev/usb/net/if_ipheth.c

623

should we handle the case that uether_newbuf() fails?

668

should we handle the case that uether_newbuf() fails?

[aokblast]

Fix comments

[lwhsu]

sys/dev/usb/net/if_ipheth.c

553–557

This was in the for (x = 0; x != IPHETH_RX_FRAMES_MAX; x++) { loop. Is it intentional to be moved out of the loop? (If so, then x == IPHETH_RX_FRAMES_MAX seems weird.)

sys/dev/usb/net/if_iphethvar.h

83

I know this builds fine as we have #include <dev/usb/usb_cdc.h> first in if_ipheth.c, however, do we need to also need #include <dev/usb/usb_cdc.h> in if_iphethvar.h for those usb_ncm16_* for completeness?

[aokblast]

Move set_frame_length inside

sys/dev/usb/net/if_iphethvar.h

83

I thought *var.h is a private header and should prevent including any header file?

[lwhsu]

sys/dev/usb/net/if_ipheth.c

632

just be curious, if this is safe to ignore this frame, do we still need to copy out and put to rxmbuf instead of just dropping it?

642

This return backs to ipheth_bulk_read_callback()'s USB_ST_TRANSFERRED right? If so, then it seems falling through to USB_ST_SETUP and there are also usbd_transfer_submit(xfer); and uether_rxflush(&sc->sc_ue);. Is this a possible issue?

[aokblast]

remove unneeded xfer submit in ncm handler

[aokblast]

sys/dev/usb/net/if_ipheth.c

632

No, but I would like to do so to let it pass through the process of network and emulated as a invalid ethernet frame.

[lwhsu]

sys/dev/usb/net/if_iphethvar.h

83

seems so, as it also doesn't include other files for usb_* I think it's fine.

[lwhsu]

I think this is fine, but let's wait others for a little while to comment.

For the commit message, I have done some adjustment. Check if this is useful for you:

ipheth(4): add CDC-NCM support for RX

The CDC-NCM (USB Communications Device Class – Network Control Model) protocol
allows multiple Ethernet frames to be encapsulated into a single USB transfer.

On iOS, CDC-NCM is currently implemented for RX only and uses a fixed number of
entries (16). To maintain compatibility with older iOS versions, we attempt to
enable NCM on the USB device first; if this fails, we fall back to the original
behavior.

sys/dev/usb/net/if_ipheth.c

236

From its usage and the return statement, perhaps the type can be bool?

[markj]

sys/dev/usb/net/if_ipheth.c

608

Why is this field in the softc, and not on the stack? It looks like it is only used in this function.

618

627

Usually it's a bad idea to put print statements in the data path. If there is a memory shortage, we may print this line many many times, and printing to the console is generally slow.

I suggest either:

• removing the print entirely (you are recording the drop in the interface counters, so it can be seen), or
• using ratecheck(9) to limit the prints to 1 per second or similar.

632

You should explain this in a comment--as a reader it's hard to see if this behaviour is intentional, or a bug.

677

This code is assuming that dp_len <= new_buf->m_len, but how is it guaranteed?

[aokblast]

Fix comments

[lwhsu]

sys/dev/usb/net/if_ipheth.c

618

[markj]

sys/dev/usb/net/if_ipheth.c

639

So, if we get the unknown 4-byte buffer above, we pass it to the ethernet stack (why?), but here we do not. Why not handle these cases consistently?

(I think passing the 4-byte buffer to the network stack is rather strange, on second thought. If such buffers are "expected", then we should handle them in the driver. Otherwise they will cause an error counter in the network stack to be incremented, and someone might waste time trying to understand why.)

[aokblast]

Fix inconsist send

[aokblast]

sys/dev/usb/net/if_ipheth.c

639

When I was look at the problem of ipheth and create this patch first time. I discover that I need to handle the special buffer above or I cannot receive any packet anymore. However, when I delete the 0x00 0x01 handler now, it still works. So we don't need the special handler now.

[markj]

sys/dev/usb/net/if_ipheth.c

651

This isn't sufficient. m_getcl() returns a buffer of size MCLBYTES, which is 2KB by default. dp_len might be larger than that, I think.

I see no reason not to use uether_newbuf(), just make sure that we drop packets that are too big.

[aokblast]

Fix potential buffer overflow

[markj]

sys/dev/usb/net/if_ipheth.c

649

It's important to increment IFCOUNTER_IQDROPS when we throw away a packet.

Why do we handle these two cases differently, i.e., break vs. continue?

[aokblast]

sys/dev/usb/net/if_ipheth.c

649

I am a little bit confused by this. According to the netlink(4), IFCOUNTER_IQDROPS means that we have a valid packet but no buffer space now. But in here it is not a valid packet. I also notice that if_cdce does not increase drop counter when dp_offset and dp_len is invalid.

[markj]

sys/dev/usb/net/if_ipheth.c

649

Well, if the packet is too large to fit in a mbuf cluster, IQDROPS is a reasonable way to report that. There is also IFCOUNTER_IERROR. The point is that we should avoid silently dropping packets, there should always be some kind of record.

IERROR is more suitable for the case where dp_len is invalid, IMO. I think if_cdce is not behaving correctly, looking at other USB NIC drivers, they are inconsistent about how errors are handled. I think it is better to record errors in this case.

[aokblast]

Record error when invalid packet and insufficient buffer happens.

[markj]

Looks ok to me aside from a couple of nits.

sys/dev/usb/net/if_ipheth.c

634

658

Shouldn't this be continue as well?

[aokblast]

Fix nits

[markj]

Approved

[aokblast]

Close because of merging in https://cgit.FreeBSD.org/src/commit/?id=c992ac6213276f54d868f317cc5092f8aed4ff54.