On 1/24/24 17:39, Aaron Conole wrote:
> Dumitru Ceara <[email protected]> writes:
>
>> On 1/23/24 00:11, Mike Pattrick wrote:
>>> The OVN test suite identified a bug in dp_packet_ol_send_prepare() where
>>> a BFD packet flagged as double encapsulated would trigger a seg fault.
>>> The problem surfaced because bfd_put_packet was reusing a packet
>>> allocated on the stack that wasn't having its flags reset between calls.
>>>
>>
>> Thanks for tracking this one down, Mike!
>>
>>> This change will reset OL flags in data_clear(), which should fix this
>>> type of packet reuse issue in general as long as data_clear() is called
>>> in between uses. This change also includes a tangentially related check
>>> in dp_packet_inner_l4_size(), where the correct offset was not being
>>> checked.
>>
>> Up to maintainers but should this tangential fix be a separate patch?
>
> I think it makes sense. These are two different issues, so probably
> should go as two separate patches.
+1
>
>>>
>>> Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.")
>>> Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.")
>>> Reported-by: Dumitru Ceara <[email protected]>
>>> Reported-at: https://issues.redhat.com/browse/FDP-300
>>> Signed-off-by: Mike Pattrick <[email protected]>
>>> ---
>>
>> I'm no expert in this code but the change itself looks correct to me and
>> OVN tests pass with this applied:
>>
>> Reviewed-by: Dumitru Ceara <[email protected]>
>>
>>> lib/dp-packet.h | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>>> index 939bec5c8..f328a6637 100644
>>> --- a/lib/dp-packet.h
>>> +++ b/lib/dp-packet.h
>>> @@ -207,6 +207,7 @@ void *dp_packet_resize_l2(struct dp_packet *, int
>>> increment);
>>> void *dp_packet_resize_l2_5(struct dp_packet *, int increment);
>>> static inline void *dp_packet_eth(const struct dp_packet *);
>>> static inline void dp_packet_reset_offsets(struct dp_packet *);
>>> +static inline void dp_packet_reset_offload(struct dp_packet *);
>>> static inline uint16_t dp_packet_l2_pad_size(const struct dp_packet *);
>>> static inline void dp_packet_set_l2_pad_size(struct dp_packet *, uint16_t);
>>> static inline void *dp_packet_l2_5(const struct dp_packet *);
>>> @@ -380,6 +381,7 @@ dp_packet_clear(struct dp_packet *b)
>>> {
>>> dp_packet_set_data(b, dp_packet_base(b));
>>> dp_packet_set_size(b, 0);
>>> + dp_packet_reset_offload(b);
Should we also reset offsets here? They are also not valid after
clearing the packet data.
Looking through the different dp-packet functions it seems we're
missing adjustments for inner offsets in dp_packet_resize_l2_5().
May be a problem if we would also need to push a vlan after
encapsulation of packet with checksum offload.
May be a separate fix as well.
>>> }
>>>
>>> /* Removes 'size' bytes from the head end of 'b', which must contain at
>>> least
>>> @@ -537,7 +539,7 @@ dp_packet_inner_l4(const struct dp_packet *b)
>>> static inline size_t
>>> dp_packet_inner_l4_size(const struct dp_packet *b)
>>> {
>>> - return OVS_LIKELY(b->l4_ofs != UINT16_MAX)
>>> + return OVS_LIKELY(b->inner_l4_ofs != UINT16_MAX)
>>> ? (const char *) dp_packet_tail(b)
>>> - (const char *) dp_packet_inner_l4(b)
>>> - dp_packet_l2_pad_size(b)
>>
>> Regards,
>> Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
