if_genet.c beginning line 1247 in function gen_parse_tx() #define COPY(size) { \ int hsize = size; \ if (copy) { \ if (shift) { \ u_char *p0; \ shift = false; \ p0 = mtodo(m0, sizeof(struct statusblock)); \ m0->m_data = m0->m_pktdat; \ bcopy(p0, mtodo(m0, sizeof(struct statusblock)),\ m0->m_len - sizeof(struct statusblock)); \ copy_p = mtodo(m0, sizeof(struct statusblock)); \ } \ bcopy(p, copy_p, hsize); \ m0->m_len += hsize; \ m0->m_pkthdr.len += hsize; /* unneeded */ \ m->m_len -= hsize; \ m->m_data += hsize; \ } \ copy_p += hsize; \ } In mbuf.h line 116, the definition of mtodo() is #define mtodo(m, o) ((void *)(((m)->m_data) + (o))) In genet, the "COPY()" macro function will copy the Link Layer Header and Network Layer Header into a contiguous memory space. There are two memory copy operations in the "COPY()" function. The first will be performed if the "shift" variable is set as true. The second memory copy operation is performed whether the first memory copy operation is performed or not. The "m0->m_len" is the data length of the original "m0->m_data". "p0" points to "m0->m_data + sizeof(statusblock)". The "m0->m_data" will then be changed to point to "m0->m_pktdat". The memory overwrite will occur when the "shift" variable is true and "m0->m_len" is larger than the "sizeof(struct statusblock)". The first "bcopy()" will copy all the contents excepting "statusblock" from the old "m0->m_data", starting at "p0", to the position starting at "m0->m_data + sizeof(struct statusblock)". The "copy_p" will be set to point to "m0->m_data + sizeof(struct statusblock)". The second "bcopy()" will copy the contents from "p" to "copy_p" which will overwrite some/all of the contents which are copied at the first copy. Should "copy_p" point to the "m0->m_data + sizeof(struct statusblock) + m0->m_len - sizeof(struct statusblock)" to prevent memory overwrite? - copy_p = mtodo(m0, sizeof(struct statusblock)); \ + copy_p = mtodo(m0, m0->m_len); \ It is a rare case that only happens when the content of a packet is located in different mbufs in a mbuf chain. It happens in my development environment when I tried to send a large ping packet, for example "ping -s 2048 .....".
In the failing case, the link-layer header is overwritten by the network-layer header? I think you are correct about the fix. I will have to figure out a way to force that case for testing.
(In reply to Mike Karels from comment #1) Yes, exactly. The link-layer header will be overwritten by the network header if they are located in two consecutive mbufs. And the "statusblock" is prepended before the link-layer header in the same mbuf.
Had you set the value of the sysctl hw.genet.tx_hdr_min to 0? I couldn't drive the code through the block with the bug without doing that. Even with that, I could only do it with a random failure in M_PREPEND. However, I tested your fix successfully and demonstrated the problem without it.
(In reply to Mike Karels from comment #3) Hi, Happy to hear that the problem can be fixed. I cannot reproduce the problem in the current release of the Freebsd image but I never try to change any parameter in "sysctl". This problem happens in my own development environment. My development environment is not entirely based on the Freebsd, but Freebsd is running within our development environment and the version of Freebsd is not based on the current release. hw.genet.tx_hdr_min does not exist in the Freebsd running in my development environment. I can provide further details to help reproduce this issue. Let's say we want to send a large packet, e.x. "ping -s 2048 ....", and the packet is going to fragmented at the network layer, IP layer. For the first fragmented packet, the network header, ICMP header and a portion of payload are stored in one mbuf, and "M_EXT" macro is set at that mbuf based on the rule in the code. Therefore, the mbuf is not writeable. The link-layer header and statusblock will be prepended to a new mbuf inserted before the mbuf carrying the "network header + ICMP header + payload". For reproducing the problem, it might not be necessary to send a large packet, but just make the mbuf not writable.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=1de9aa4d4f7938f36e6485dad817908a6e45bb32 commit 1de9aa4d4f7938f36e6485dad817908a6e45bb32 Author: Mike Karels <karels@FreeBSD.org> AuthorDate: 2022-05-09 12:19:52 +0000 Commit: Mike Karels <karels@FreeBSD.org> CommitDate: 2022-05-09 13:46:06 +0000 genet: fix output packet corruption in uncommon case The code for the "shift" block in the COPY macro set the pointer for the next copy block to the wrong value. In this case, the link-layer header would be overwritten by the network-layer header. This case is difficult or impossible to exercise in the current driver without changing the value of the hw.genet.tx_hdr_min sysctl. Correct the pointer. While here, remove a line in the macro that was marked "unneeded", which was actually wrong. PR: 263824 Submitted by: jiahali@blackberry.com MFC after: 2 weeks sys/arm64/broadcom/genet/if_genet.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
(In reply to Jiahao LI from comment #4) > hw.genet.tx_hdr_min does not exist in the Freebsd running in my development environment. Ah, then it is an older version of the driver; the sysctl was added there last June. The version without the sysctl would act as if the sysctl value was 0 for the IPv4 case. Should be fixed in the version I just checked in. Marking for MFC to 13, but not 12; the driver is not in 12.
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=7e6e22aab6b993e42328bafe0f64ee14a2b7c43c commit 7e6e22aab6b993e42328bafe0f64ee14a2b7c43c Author: Mike Karels <karels@FreeBSD.org> AuthorDate: 2022-05-09 12:19:52 +0000 Commit: Mike Karels <karels@FreeBSD.org> CommitDate: 2022-05-23 11:53:01 +0000 genet: fix output packet corruption in uncommon case The code for the "shift" block in the COPY macro set the pointer for the next copy block to the wrong value. In this case, the link-layer header would be overwritten by the network-layer header. This case is difficult or impossible to exercise in the current driver without changing the value of the hw.genet.tx_hdr_min sysctl. Correct the pointer. While here, remove a line in the macro that was marked "unneeded", which was actually wrong. PR: 263824 Submitted by: jiahali@blackberry.com (cherry picked from commit 1de9aa4d4f7938f36e6485dad817908a6e45bb32) sys/arm64/broadcom/genet/if_genet.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Fixed on main and stable/13