On Thu, Aug 28, 2025 at 02:15:53PM +0200, Florian Westphal wrote: > 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(). There are a more checks for ct->status & NAT_MASK in the tree that I can see, if you are correct, then maybe a new helper function to check for NAT_MASK is needed. Anyway, as for this patch, READ_ONCE should not harm.
