linux-kernel - Re: [PATCH 01/41] rxrpc: Fix RTT determination to use PING ACKs as a source

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c19af528-1aad-412c-8362-275c791dd76f@auristor.com>
Date:   Fri, 10 Nov 2023 09:15:31 -0500
From:   Jeffrey E Altman <jaltman@...istor.com>
To:     David Howells <dhowells@...hat.com>
Cc:     Marc Dionne <marc.dionne@...istor.com>,
        linux-afs@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/41] rxrpc: Fix RTT determination to use PING ACKs as a
 source

On 11/9/2023 5:06 PM, David Howells wrote:
> I do not believe the ack_reason matters within rxrpc_input_ack(). As 
> long as
>> the acked_serial is non-zero,
>> rxrpc_complete_rtt_probe() can be called to attempt to compute an RTT.   If
>> there is an exact match for the
>> acked_serial then an RTT can be computed and if acked_serial is later than the
>> pending rtt probe, the probe
>> can be abandoned with the following caveats.
>>
>> 1. Receiving an acked_serial that is later than the serial of the
>>     transmitted probe indicates that a packet
>>     transmitted after the probe was received first.  Or that reordering
>>     of the transmitted packets occurred.
>>     Or that the probe was never received by the peer; or that the peer's
>>     response to the probe was lost in
>>     transit.
>> 2. The serial number namespace is unsigned 32-bit shared across all of
>>     the call channels of the associated
>>     rx connection.  As the serial numbers will wrap the use of after()
>>     within rxrpc_complete_rtt_probe to
>>     compare their values is questionable.   If serial numbers will be
>>     compared in this manner then they
>>     need to be locally tracked and compared as unsigned 64-bit values
>>     where only the low 32-bits are
>>     transmitted on the wire and any wire serial number equal to zero is
>>     ignored.
> I do ignore ack.serial == 0 for this purpose.

Zero has the special meaning - this ACK is not explicitly in response to 
a received packet.

However, as mentioned, the serial number counter wraps frequently and 
most RxRPC implementations
do not transition from serial 0xffffffff -> 0x00000001 when wrapping.

> I'm not sure how expanding it internally to 64-bits actually helps since the
> upper 32 bits is not visible to the peer.
rxrpc_complete_rtt_probe contains the following logic that relies on 
after() being able to detect
a serial number wrap.

         /* If a later serial is being acked, then mark this slot as
          * being available.
          */

         if (after(acked_serial, orig_serial)) {
             trace_rxrpc_rtt_rx(call, rxrpc_rtt_rx_obsolete, i,
                        orig_serial, acked_serial, 0, 0);
             clear_bit(i + RXRPC_CALL_RTT_PEND_SHIFT, &call->rtt_avail);
             smp_wmb();
             set_bit(i, &call->rtt_avail);
         }

Otherwise, acked_serial = 0x01 will be considered smaller than 
orig_serial = 0xfffffffe and the slot will not be marked available.

I will note that there is a similar problem with rxrpc_seq_t values 
which are u32 on the wire but which will wrap for calls that transmit 
more than approximately 5.5TB of data. Calls of this size are unlikely 
for a cache manager but are common for any service transmitting volume 
dumps.

Jeffrey Altman


Download attachment "smime.p7s" of type "application/pkcs7-signature" (4039 bytes)