| Message ID | 20240122231144.574781-1-mkp@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] dp-packet: Reset offload flags when clearing a packet. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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? > > Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.") > Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.") > Reported-by: Dumitru Ceara <dceara@redhat.com> > Reported-at: https://issues.redhat.com/browse/FDP-300 > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- 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 <dceara@redhat.com> > 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); > } > > /* 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
Dumitru Ceara <dceara@redhat.com> 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. >> >> Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.") >> Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.") >> Reported-by: Dumitru Ceara <dceara@redhat.com> >> Reported-at: https://issues.redhat.com/browse/FDP-300 >> Signed-off-by: Mike Pattrick <mkp@redhat.com> >> --- > > 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 <dceara@redhat.com> > >> 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); >> } >> >> /* 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 1/24/24 17:39, Aaron Conole wrote: > Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> >>> Reported-at: https://issues.redhat.com/browse/FDP-300 >>> Signed-off-by: Mike Pattrick <mkp@redhat.com> >>> --- >> >> 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 <dceara@redhat.com> >> >>> 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
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); } /* 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)
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. 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. Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.") Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.") Reported-by: Dumitru Ceara <dceara@redhat.com> Reported-at: https://issues.redhat.com/browse/FDP-300 Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/dp-packet.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)