[API-NEXT,PATCHv2] linux-generic: packet: avoid race condition in packet_free processing

Message ID 20170218125225.26824-1-bill.fischofer@linaro.org
State New
Headers show

Commit Message

Bill Fischofer Feb. 18, 2017, 12:52 p.m.
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

Comments

Peltonen, Janne (Nokia - FI/Espoo) Feb. 20, 2017, 8:13 p.m. | #1
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
Bill Fischofer Feb. 22, 2017, 4:16 p.m. | #2
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

>

>

Patch hide | download patch | download mbox

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;