The m_copym() function and its historic alias m_copy() are used all over the kernel to get a copy of a mbuf chain. However, at least some code fails to account for the fact that the copy is read-only due to the mbuf clusters from the original chain not actually copied, but merely referenced by the new chain. The most obvious case is the routines calling if_simloop() to return a copy of a network packet up the stack. It is done to let the stack see its own broadcast traffic if the outgoing interface is simplex. Such routines all use m_copym() to make the copy, but the upper layers of the stack are not prohibited from modifying the mbuf chain they get on input. Consequently, the original chain ready to be transmitted to the network may be damaged unpredictably. The damage can have various consequences, from broken packets sent to system crashes. In addition, there might be more places in the kernel where m_copym() or m_copy() is used although a writable copy is needed. Fix: The initial idea is to drop all the bogus and often buggy code around if_simloop() calls and add a flag to if_simloop() indicating that the mbuf chain must be fully duplicated with m_dup() and then the copy is to be processed. In STABLE, the flag can be indicated by adding M_COPYALL to the hlen argument. M_COPYALL is a very large value that allows for the trick. So ABI will be preserved in STABLE. At the same time, the other cases of m_copym() usage should be revised thoroughly. How-To-Repeat: Although the problem didn't manifested itself for ages, a recent change in the distribution of data over a mbuf chain has finally triggered it in CURRENT. A stock generator of broadcast traffic is rwhod(8). As soon as its packet is longer than ~256 bytes and so it doesn't fit in one simple mbuf, it appears damaged on the network. To reach the threshold length, just open multiple terminal sessions to the system. 16:15:28.212810 IP truncated-ip - 6865 bytes missing! (tos 0x0, ttl 64, id 28554, offset 0, \ flags [none], proto: UDP (17), length: 7169, bad cksum 11c (->c64b)!) \ 10.10.10.4.513 > 10.10.10.255.513: UDP, length 276 0x0000: 4500 1c01 6f8a 0000 4011 011c 0a0a 0a04 E...o...@....... ^^^^ ^^^^ broken fields 0x0010: 0a0a 0aff 0201 0201 011c 0000 0101 0000 ................ 0x0020: 4565 9ef0 0000 0000 6467 0000 0000 0000 Ee......dg...... 0x0030: 0000 0000 0000 0000 0000 0000 0000 0000 ................ 0x0040: 0000 0000 0000 0000 0000 001a 0000 000e ................ 0x0050: 0000 0005 4564 a5e7 7474 7976 3000 0000 ....Ed..ttyv0... 0x0060: 726f 6f74 0000 0000 4565 9d4a 0000 01a6 root....Ee.J.... 0x0070: 7474 7976 3100 0000 726f 6f74 0000 0000 ttyv1...root.... 0x0080: 4565 9d4d 0000 000c 7474 7976 3200 0000 Ee.M....ttyv2... 0x0090: 726f 6f74 0000 0000 4565 9d4f 0000 0099 root....Ee.O.... 0x00a0: 7474 7976 3300 0000 726f 6f74 0000 0000 ttyv3...root.... 0x00b0: 4565 9d52 0000 019e 7474 7976 3400 0000 Ee.R....ttyv4... 0x00c0: 726f 6f74 0000 0000 4565 9d54 0000 019c root....Ee.T.... 0x00d0: 7474 7976 3500 0000 726f 6f74 0000 0000 ttyv5...root.... 0x00e0: 4565 9d59 0000 0198 7474 7976 3600 0000 Ee.Y....ttyv6... 0x00f0: 726f 6f74 0000 0000 4565 9d5b 0000 0195 root....Ee.[.... 0x0100: 7474 7976 3700 0000 726f 6f74 0000 0000 ttyv7...root.... 0x0110: 4565 9d5e 0000 0000 7474 7970 3100 0000 Ee.^....ttyp1... 0x0120: 7961 7200 0000 0000 4565 8361 0000 04b2 yar.....Ee.a....
yar 2006-12-24 08:52:13 UTC FreeBSD src repository Modified files: sys/net if_ethersubr.c Log: Note that rev. 1.221 introduced a local workaround for a general problem. Add a pointer to the relevant PR for future reference. The whole comment will be OK to remove as soon as the general solution is applied. PR: kern/105943 Revision Changes Path 1.222 +4 -0 src/sys/net/if_ethersubr.c _______________________________________________ cvs-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/cvs-all To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
JFTR: A local workaround was independently applied to if_ethersubr.c in its rev. 1.221. However, less used L2 modules such as arcnet, fddi, and token ring still suffer from the same bug. Moreover, as already noted in this PR, other callers of m_copym() may get it wrong, too. Unfortunately, I neither suffer from this issue personally nor have free time to fully investigate and address it out of general interest. OTOH, financial support can motivate me to do so. Granting the number of source files potentially involved (~45) and the amount of thorough testing requred, it would be not less than 10 hours worth of work. -- Yar
State Changed From-To: open->suspended A workaround has been committed, but a general solution has not yet been committed. Awaiting an interested person.
Responsible Changed From-To: freebsd-bugs->freebsd-net This should probably be assigned to -net.
batch change: For bugs that match the following - Status Is In progress AND - Untouched since 2018-01-01. AND - Affects Base System OR Documentation DO: Reset to open status. Note: I did a quick pass but if you are getting this email it might be worthwhile to double check to see if this bug ought to be closed.
Git hash for that comment: 9983b3c02def80c27bf3ca0284fba2537d932f4b I don't think it's really a local workaround though -- if a writable mbuf is needed, m_dup is the correct call. The "general solution" is to use m_dup when it is necessary to do so. m_copy was removed in: commit c3bef61e584084a8f86fba71cb344f15fc20491c Author: Kevin Lo <kevlo@FreeBSD.org> Date: Thu Sep 15 07:41:48 2016 +0000 Remove the 4.3BSD compatible macro m_copy(), use m_copym() instead. Reviewed by: gnn Differential Revision: https://reviews.freebsd.org/D7878 There are 81 instances of "m_copym" in the tree still, across 41 files. > less used L2 modules such as arcnet, fddi, and token ring still suffer from the same bug. These are all gone.
ip_mloopback: /* * Make a deep copy of the packet because we're going to * modify the pack in order to generate checksums. */ copym = m_dup(m, M_NOWAIT); ... if_simloop(ifp, copym, AF_INET, 0); pim6_input: mcp = m_copym(m, 0, off + PIM6_REG_MINLEN, M_NOWAIT); ... if_simloop(mif6table[reg_mif_num].m6_ifp, m, dst.sin6_family, 0); ip6_mloopback: copym = m_copym(m, 0, M_COPYALL, M_NOWAIT); ... /* * Make sure to deep-copy IPv6 header portion in case the data * is in an mbuf cluster, so that we can safely override the IPv6 * header portion later. */ if (!M_WRITABLE(copym) || copym->m_len < sizeof(struct ip6_hdr)) { copym = m_pullup(copym, sizeof(struct ip6_hdr)); if (copym == NULL) return; } ... if_simloop(ifp, copym, AF_INET6, 0);