| Message ID | 1465498232-96859-1-git-send-email-jesse@kernel.org |
|---|---|
| State | Accepted |
| Headers | show |
[Google filtered by ack as spam, so I'm trying again.] Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Jun 9, 2016, at 11:50 AM, Jesse Gross <jesse@kernel.org> wrote: > > Since tunnel metadata is stored in a fixed area in the flow match field, we > must allocate space for options as they are registered with the switch. In order > to avoid exposing implementation complexity to the controller, we support > fragmentation when we run out of contiguous blocks that are large enough to > handle new requests. > > When reading or writing to these fragmented blocks, there is a bug that would > cause us to keep on using the area after the allocated space rather than moving > to the next offset. This corrects that to use the offset for each block. > > Unfortunately, while we did have a test for this exact use case, since the same > bug was present in both reading and writing code, everything appeared to work > as normal from the outside. > > Signed-off-by: Jesse Gross <jesse@kernel.org> > --- > lib/tun-metadata.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c > index a773aea..7a2a84f 100644 > --- a/lib/tun-metadata.c > +++ b/lib/tun-metadata.c > @@ -490,7 +490,7 @@ memcpy_to_metadata(struct tun_metadata *dst, const void *src, > int addr = 0; > > while (chain) { > - memcpy(dst->opts.u8 + loc->c.offset + addr, (uint8_t *)src + addr, > + memcpy(dst->opts.u8 + chain->offset, (uint8_t *)src + addr, > chain->len); > addr += chain->len; > chain = chain->next; > @@ -507,7 +507,7 @@ memcpy_from_metadata(void *dst, const struct tun_metadata *src, > int addr = 0; > > while (chain) { > - memcpy((uint8_t *)dst + addr, src->opts.u8 + loc->c.offset + addr, > + memcpy((uint8_t *)dst + addr, src->opts.u8 + chain->offset, > chain->len); > addr += chain->len; > chain = chain->next; > -- > 2.5.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Thanks for the review, I applied to this master and branch-2.5. On Tue, Jun 21, 2016 at 10:08 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > [Google filtered by ack as spam, so I'm trying again.] > > Acked-by: Jarno Rajahalme <jarno@ovn.org> > >> On Jun 9, 2016, at 11:50 AM, Jesse Gross <jesse@kernel.org> wrote: >> >> Since tunnel metadata is stored in a fixed area in the flow match field, we >> must allocate space for options as they are registered with the switch. In order >> to avoid exposing implementation complexity to the controller, we support >> fragmentation when we run out of contiguous blocks that are large enough to >> handle new requests. >> >> When reading or writing to these fragmented blocks, there is a bug that would >> cause us to keep on using the area after the allocated space rather than moving >> to the next offset. This corrects that to use the offset for each block. >> >> Unfortunately, while we did have a test for this exact use case, since the same >> bug was present in both reading and writing code, everything appeared to work >> as normal from the outside. >> >> Signed-off-by: Jesse Gross <jesse@kernel.org> >> --- >> lib/tun-metadata.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c >> index a773aea..7a2a84f 100644 >> --- a/lib/tun-metadata.c >> +++ b/lib/tun-metadata.c >> @@ -490,7 +490,7 @@ memcpy_to_metadata(struct tun_metadata *dst, const void *src, >> int addr = 0; >> >> while (chain) { >> - memcpy(dst->opts.u8 + loc->c.offset + addr, (uint8_t *)src + addr, >> + memcpy(dst->opts.u8 + chain->offset, (uint8_t *)src + addr, >> chain->len); >> addr += chain->len; >> chain = chain->next; >> @@ -507,7 +507,7 @@ memcpy_from_metadata(void *dst, const struct tun_metadata *src, >> int addr = 0; >> >> while (chain) { >> - memcpy((uint8_t *)dst + addr, src->opts.u8 + loc->c.offset + addr, >> + memcpy((uint8_t *)dst + addr, src->opts.u8 + chain->offset, >> chain->len); >> addr += chain->len; >> chain = chain->next; >> -- >> 2.5.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >
diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c index a773aea..7a2a84f 100644 --- a/lib/tun-metadata.c +++ b/lib/tun-metadata.c @@ -490,7 +490,7 @@ memcpy_to_metadata(struct tun_metadata *dst, const void *src, int addr = 0; while (chain) { - memcpy(dst->opts.u8 + loc->c.offset + addr, (uint8_t *)src + addr, + memcpy(dst->opts.u8 + chain->offset, (uint8_t *)src + addr, chain->len); addr += chain->len; chain = chain->next; @@ -507,7 +507,7 @@ memcpy_from_metadata(void *dst, const struct tun_metadata *src, int addr = 0; while (chain) { - memcpy((uint8_t *)dst + addr, src->opts.u8 + loc->c.offset + addr, + memcpy((uint8_t *)dst + addr, src->opts.u8 + chain->offset, chain->len); addr += chain->len; chain = chain->next;
Since tunnel metadata is stored in a fixed area in the flow match field, we must allocate space for options as they are registered with the switch. In order to avoid exposing implementation complexity to the controller, we support fragmentation when we run out of contiguous blocks that are large enough to handle new requests. When reading or writing to these fragmented blocks, there is a bug that would cause us to keep on using the area after the allocated space rather than moving to the next offset. This corrects that to use the offset for each block. Unfortunately, while we did have a test for this exact use case, since the same bug was present in both reading and writing code, everything appeared to work as normal from the outside. Signed-off-by: Jesse Gross <jesse@kernel.org> --- lib/tun-metadata.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)