Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote: > On Thu, Aug 28, 2025 at 11:14:35AM +0200, Fabian Bläse wrote: > > The icmp_ndo_send function was originally introduced to ensure proper > > rate limiting when icmp_send is called by a network device driver, > > where the packet's source address may have already been transformed > > by SNAT. > > > > However, the original implementation only considers the > > IP_CT_DIR_ORIGINAL direction for SNAT and always replaced the packet's > > source address with that of the original-direction tuple. This causes > > two problems: > > > > 1. For SNAT: > > Reply-direction packets were incorrectly translated using the source > > address of the CT original direction, even though no translation is > > required. > > > > 2. For DNAT: > > Reply-direction packets were not handled at all. In DNAT, the original > > direction's destination is translated. Therefore, in the reply > > direction the source address must be set to the reply-direction > > source, so rate limiting works as intended. > > > > Fix this by using the connection direction to select the correct tuple > > for source address translation, and adjust the pre-checks to handle > > reply-direction packets in case of DNAT. > > > > Additionally, wrap the `ct->status` access in READ_ONCE(). This avoids > > possible KCSAN reports about concurrent updates to `ct->status`. > > I think such concurrent update cannot not happen, NAT bits are only > set for the first packet of a connection, which sets up the nat > configuration, so READ_ONCE() can go away. Yes, the NAT bits stay in place but not other flags in ->status, e.g. DYING, ASSURED, etc. So I believe its needed, concurrent update of ->status is possible and KCSAN would warn. Other spots either use READ_ONCE or use test_bit().
