[ovs-dev] [PATCH] ofp-parse: Correctly update bucket lists if they are empty

Simon Horman simon.horman at netronome.com
Mon Jan 26 08:30:47 UTC 2015


On Mon, Jan 26, 2015 at 04:40:49PM +0900, Simon Horman wrote:
> list_moved() appears to only work reliably if its argument is a non-empty
> list. This is because list_moved dereferences the next and prev fields
> of the list, which for an empty list are the address of the list itself,
> and list_moved() is intended to be called when the address of the list has
> changed.
> 
> This patch updates the only user of list_moved() that I was able to find
> my temporarily setting the next field to NULL if it is empty and then
> re-initialising the list rather than calling list_moved() after
> reallocation has occurred.
> 
> An test update is also provided to exercise this change. Without
> the code change the updated test cases a segmentation when the
> buckets parameter of ofputil_put_ofp11_bucket() is dereferenced.
> 
> Signed-off-by: Simon Horman <simon.horman at netronome.com>
> 
> ---
> 
> * Although somewhat cure the approach of setting the next field to NULL

s/cure/crude/

>   seems far less dangerous than trying to update the list infrastructure
>   to handle this case.
> 
>   I also toyed with using a temporary bitmap to track empty buckets.
>   That approach worked but seemed to be more overhead and no cleaner
>   than the one taken by this patch.
> ---
>  lib/ofp-parse.c  | 11 ++++++++++-
>  tests/ofproto.at | 13 +++++++++++--
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 9acf6a4..9a5df3b 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -1415,9 +1415,18 @@ parse_ofp_group_mod_file(const char *file_name, uint16_t command,
>          if (*n_gms >= allocated_gms) {
>              size_t i;
>  
> +            for (i = 0; i < *n_gms; i++) {
> +                if (list_is_empty(&(*gms)[i].buckets)) {
> +                    (*gms)[i].buckets.next = NULL;
> +                }
> +            }
>              *gms = x2nrealloc(*gms, &allocated_gms, sizeof **gms);
>              for (i = 0; i < *n_gms; i++) {
> -                list_moved(&(*gms)[i].buckets);
> +                if ((*gms)[i].buckets.next) {
> +                    list_moved(&(*gms)[i].buckets);
> +                } else {
> +                    list_init(&(*gms)[i].buckets);
> +                }
>              }
>          }
>          error = parse_ofp_group_mod_str(&(*gms)[*n_gms], command, ds_cstr(&s),
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 2a2111f..39978e4 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -286,16 +286,25 @@ dnl Actions definition listed in both supported formats (w/ actions=)
>  AT_SETUP([ofproto - del group (OpenFlow 1.1)])
>  OVS_VSWITCHD_START
>  AT_DATA([groups.txt], [dnl
> +group_id=1231,type=all
> +group_id=1232,type=all
> +group_id=1233,type=all
>  group_id=1234,type=all,bucket=output:10
>  group_id=1235,type=all,bucket=actions=output:10
>  ])
>  AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-groups br0 groups.txt])
>  AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn dump-groups br0 ], [0], [stdout])
> -AT_CHECK([STRIP_XIDS stdout], [0], [dnl
> -OFPST_GROUP_DESC reply (OF1.1):
> +AT_CHECK([STRIP_XIDS stdout | sort], [0], [dnl
> + group_id=1231,type=all
> + group_id=1232,type=all
> + group_id=1233,type=all
>   group_id=1234,type=all,bucket=actions=output:10
>   group_id=1235,type=all,bucket=actions=output:10
> +OFPST_GROUP_DESC reply (OF1.1):
>  ])
> +AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1231])
> +AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1232])
> +AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1233])
>  AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1234])
>  AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn dump-groups br0], [0], [stdout])
>  AT_CHECK([STRIP_XIDS stdout], [0], [dnl
> -- 
> 2.1.4
> 



More information about the dev mailing list