| Message ID | 20170218125225.26824-1-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, I do not think this really fixes the problem. If we ignore memory ordering issues for now, I think this would fix the problem that unshared_len would be modified after the packet has been freed (since now the current thread keeps its own reference over the unshared_len modification). However, unshared_len may still be set to frame_len when the packet is actually shared, because checking the ref count and then adjusting unshared_len is not atomic. If packet_ref_dec() returns 1 then the current thread must be the only one who has a reference to the the packet header. It is thus safe to assume that the ref count stays as one, as there is no other thread that could be concurrently incrementing it. But if packet_ref_dec() returns greater than 1, then another thread may have a reference to the packet hdr. At any moment the other thread may create a new reference, incrementing the reference count of the base packet. Therefore the reference count returned by packet_ref_dec() may already be old and incorrect when the caller gets it. IOW, this is always racy: > + if (ref_count == 2) > + pkt_hdr->unshared_len = pkt_hdr->frame_len; ref_count of the pkt_hdr may be exactly one just before the assignment, but already e.g. three (implying that pkt_hdr is shared by more than one other user) when the assignment takes place. Janne > -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Bill Fischofer > Sent: Saturday, February 18, 2017 2:52 PM > To: lng-odp@lists.linaro.org > Subject: [lng-odp] [API-NEXT PATCHv2] linux-generic: packet: avoid race condition in > packet_free processing > > Resolve bug https://bugs.linaro.org/show_bug.cgi?id=2892 by setting > unshared_len prior to potentially freeing segments. > > Reported-by: Janne Peltonen <janne.peltonen@nokia.com> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/odp_packet.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c > index 81bbcedd..b18b69a9 100644 > --- a/platform/linux-generic/odp_packet.c > +++ b/platform/linux-generic/odp_packet.c > @@ -658,6 +658,9 @@ static inline odp_packet_hdr_t *packet_free_to_list(odp_packet_hdr_t > *pkt_hdr, > if (*nfree + num_seg >= nbufs) > break; > > + if (ref_count == 2) > + pkt_hdr->unshared_len = pkt_hdr->frame_len; > + > for (i = 0; i < num_seg; i++) { > odp_packet_hdr_t *hdr = > pkt_hdr->buf_hdr.seg[i].hdr; > @@ -666,9 +669,6 @@ static inline odp_packet_hdr_t *packet_free_to_list(odp_packet_hdr_t > *pkt_hdr, > packet_ref_dec(hdr) == 1) > buf[(*nfree)++] = buffer_handle(hdr); > } > - > - if (ref_count == 2) > - pkt_hdr->unshared_len = pkt_hdr->frame_len; > } > > pkt_hdr = ref_hdr; > @@ -693,10 +693,10 @@ static inline void packet_free(odp_packet_hdr_t *pkt_hdr) > buffer_free_multi((odp_buffer_t *) > &pkt_hdr->buf_hdr.handle.handle, 1); > } else { > - free_bufs(pkt_hdr, 0, num_seg); > - > if (ref_count == 2) > pkt_hdr->unshared_len = pkt_hdr->frame_len; > + > + free_bufs(pkt_hdr, 0, num_seg); > } > > pkt_hdr = ref_hdr; > -- > 2.12.0.rc1
Thanks. I've revamped the whole approach to dealing with unshared_len so that this variable is only updated by the owner of the odp_packet_t that contains it. This should be faster in non-reference paths and should also eliminate all race conditions since no two threads can own the same odp_packet_t at the same time. I'll be posting a revised patch series as soon as all of Petri's packet updates have been merged and v1.14 has stabilized. Thanks again for your review help with this. On Mon, Feb 20, 2017 at 2:13 PM, Peltonen, Janne (Nokia - FI/Espoo) < janne.peltonen@nokia.com> wrote: > > Hi, > > I do not think this really fixes the problem. > > If we ignore memory ordering issues for now, I think this would fix the > problem that unshared_len would be modified after the packet has been > freed (since now the current thread keeps its own reference over the > unshared_len modification). > > However, unshared_len may still be set to frame_len when the packet > is actually shared, because checking the ref count and then adjusting > unshared_len is not atomic. > > If packet_ref_dec() returns 1 then the current thread must be the only > one who has a reference to the the packet header. It is thus safe to > assume that the ref count stays as one, as there is no other thread that > could be concurrently incrementing it. But if packet_ref_dec() returns > greater than 1, then another thread may have a reference to the > packet hdr. At any moment the other thread may create a new reference, > incrementing the reference count of the base packet. Therefore the > reference count returned by packet_ref_dec() may already be old > and incorrect when the caller gets it. > > IOW, this is always racy: > > > + if (ref_count == 2) > > + pkt_hdr->unshared_len = pkt_hdr->frame_len; > > ref_count of the pkt_hdr may be exactly one just before the > assignment, but already e.g. three (implying that pkt_hdr is > shared by more than one other user) when the assignment takes > place. > > Janne > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Bill Fischofer > > Sent: Saturday, February 18, 2017 2:52 PM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [API-NEXT PATCHv2] linux-generic: packet: avoid race > condition in > > packet_free processing > > > > Resolve bug https://bugs.linaro.org/show_bug.cgi?id=2892 by setting > > unshared_len prior to potentially freeing segments. > > > > Reported-by: Janne Peltonen <janne.peltonen@nokia.com> > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > platform/linux-generic/odp_packet.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/platform/linux-generic/odp_packet.c > b/platform/linux-generic/odp_packet.c > > index 81bbcedd..b18b69a9 100644 > > --- a/platform/linux-generic/odp_packet.c > > +++ b/platform/linux-generic/odp_packet.c > > @@ -658,6 +658,9 @@ static inline odp_packet_hdr_t > *packet_free_to_list(odp_packet_hdr_t > > *pkt_hdr, > > if (*nfree + num_seg >= nbufs) > > break; > > > > + if (ref_count == 2) > > + pkt_hdr->unshared_len = pkt_hdr->frame_len; > > + > > for (i = 0; i < num_seg; i++) { > > odp_packet_hdr_t *hdr = > > pkt_hdr->buf_hdr.seg[i].hdr; > > @@ -666,9 +669,6 @@ static inline odp_packet_hdr_t > *packet_free_to_list(odp_packet_hdr_t > > *pkt_hdr, > > packet_ref_dec(hdr) == 1) > > buf[(*nfree)++] = > buffer_handle(hdr); > > } > > - > > - if (ref_count == 2) > > - pkt_hdr->unshared_len = pkt_hdr->frame_len; > > } > > > > pkt_hdr = ref_hdr; > > @@ -693,10 +693,10 @@ static inline void packet_free(odp_packet_hdr_t > *pkt_hdr) > > buffer_free_multi((odp_buffer_t *) > > &pkt_hdr->buf_hdr.handle.handle, > 1); > > } else { > > - free_bufs(pkt_hdr, 0, num_seg); > > - > > if (ref_count == 2) > > pkt_hdr->unshared_len = pkt_hdr->frame_len; > > + > > + free_bufs(pkt_hdr, 0, num_seg); > > } > > > > pkt_hdr = ref_hdr; > > -- > > 2.12.0.rc1 > >
diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c index 81bbcedd..b18b69a9 100644 --- a/platform/linux-generic/odp_packet.c +++ b/platform/linux-generic/odp_packet.c @@ -658,6 +658,9 @@ static inline odp_packet_hdr_t *packet_free_to_list(odp_packet_hdr_t *pkt_hdr, if (*nfree + num_seg >= nbufs) break; + if (ref_count == 2) + pkt_hdr->unshared_len = pkt_hdr->frame_len; + for (i = 0; i < num_seg; i++) { odp_packet_hdr_t *hdr = pkt_hdr->buf_hdr.seg[i].hdr; @@ -666,9 +669,6 @@ static inline odp_packet_hdr_t *packet_free_to_list(odp_packet_hdr_t *pkt_hdr, packet_ref_dec(hdr) == 1) buf[(*nfree)++] = buffer_handle(hdr); } - - if (ref_count == 2) - pkt_hdr->unshared_len = pkt_hdr->frame_len; } pkt_hdr = ref_hdr; @@ -693,10 +693,10 @@ static inline void packet_free(odp_packet_hdr_t *pkt_hdr) buffer_free_multi((odp_buffer_t *) &pkt_hdr->buf_hdr.handle.handle, 1); } else { - free_bufs(pkt_hdr, 0, num_seg); - if (ref_count == 2) pkt_hdr->unshared_len = pkt_hdr->frame_len; + + free_bufs(pkt_hdr, 0, num_seg); } pkt_hdr = ref_hdr;
Resolve bug https://bugs.linaro.org/show_bug.cgi?id=2892 by setting unshared_len prior to potentially freeing segments. Reported-by: Janne Peltonen <janne.peltonen@nokia.com> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/odp_packet.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) -- 2.12.0.rc1