On Tue, Dec 7, 2021 at 8:01 AM Frode Nordahl
<frode.nord...@canonical.com> wrote:
>
> The netlink policy unit test contains test fixture data that is
> subject to endianness and currently fails on big endian systems.
>
> Store the fixture data in a struct to ensure proper byte order for
> the header data.
>
> Also fix improper style for sizeof with expressions.
>
> Fixes: bfee9f6c0115 ("netlink: Add support for parsing link layer address.")
> Signed-off-by: Frode Nordahl <frode.nord...@canonical.com>
> ---
> tests/test-netlink-policy.c | 75 ++++++++++++++++++++++++-------------
> 1 file changed, 50 insertions(+), 25 deletions(-)
>
> diff --git a/tests/test-netlink-policy.c b/tests/test-netlink-policy.c
> index 5f2bf7101..e908d3100 100644
> --- a/tests/test-netlink-policy.c
> +++ b/tests/test-netlink-policy.c
> @@ -24,6 +24,12 @@
> #include "ovstest.h"
> #include "util.h"
>
> +struct nlattr_fixture {
> + struct nlattr nlattr;
> + uint8_t data[32];
> + size_t data_len;
When I wrote this I had a very good use case for handling the whole of
the nlattr and data size separately in my head, but when revisiting it
becomes clear that the unit tests currently present in this file are
focusing on testing OVS netlink library's ability to parse a single
data type. We will not be testing the OVS netlink library's ability
to detect and handle unexpected or malformed data being transmitted
over the netlink socket, so let's scrap it as there is no real need
for it.
As a consequence a v4 will follow, sorry about the noise :-)
--
Frode Nordahl
> +};
> +
> /* nla_len is an inline function in the kernel net/netlink header, which we
> * don't necessarilly have at build time, so provide our own with
> * non-conflicting name. */
> @@ -32,66 +38,85 @@ _nla_len(const struct nlattr *nla) {
> return nla->nla_len - NLA_HDRLEN;
> }
>
> +#define TEST_POLICY_ATTR 42
> +
> static void
> test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) {
> struct nl_policy policy[] = {
> - [42] = { .type = NL_A_LL_ADDR,
> - .optional = false, },
> + [TEST_POLICY_ATTR] = { .type = NL_A_LL_ADDR,
> + .optional = false, },
> };
> struct nlattr *attrs[ARRAY_SIZE(policy)];
> - uint8_t fixture_nl_data_policy_short[] = {
> + struct nlattr_fixture fixture_nl_data_policy_short = {
> /* too short according to policy */
> - 0x04, 0x00, 0x2a, 0x00,
> + .nlattr.nla_len = 4,
> + .nlattr.nla_type = TEST_POLICY_ATTR,
> + .data = { 0x00 },
> + .data_len = 1,
> };
> - uint8_t fixture_nl_data_policy_long[] = {
> + struct nlattr_fixture fixture_nl_data_policy_long = {
> /* too long according to policy */
> - 0x19, 0x00, 0x2a, 0x00, 0x00, 0x00, 0x67, 0xfe, 0x80, 0x00, 0x00,
> 0x00,
> - 0x00, 0x00, 0x00, 0xe4, 0x1d, 0x2d, 0x03, 0x00, 0xa5, 0xf0, 0x2f,
> 0x00,
> - 0x00,
> + .nlattr.nla_len = 25,
> + .nlattr.nla_type = TEST_POLICY_ATTR,
> + .data = { 0x00, 0x00, 0x67, 0xfe, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0xe4, 0x1d, 0x2d, 0x03, 0x00, 0xa5, 0xf0, 0x2f, 0x00,
> + 0x00 },
> + .data_len = 21,
> };
> - uint8_t fixture_nl_data_eth[] = {
> + struct nlattr_fixture fixture_nl_data_eth = {
> /* valid policy and eth_addr length */
> - 0x0a, 0x00, 0x2a, 0x00, 0x00, 0x53, 0x00, 0x00, 0x00, 0x2a,
> + .nlattr.nla_len = 10,
> + .nlattr.nla_type = TEST_POLICY_ATTR,
> + .data = { 0x00, 0x53, 0x00, 0x00, 0x00, 0x2a },
> + .data_len = 6,
> };
> - uint8_t fixture_nl_data_ib[] = {
> + struct nlattr_fixture fixture_nl_data_ib = {
> /* valid policy and ib_addr length */
> - 0x18, 0x00, 0x2a, 0x00, 0x00, 0x00, 0x00, 0x67, 0xfe, 0x80, 0x00,
> 0x00,
> - 0x00, 0x00, 0x00, 0x00, 0xe4, 0x1d, 0x2d, 0x03, 0x00, 0xa5, 0xf0,
> 0x2f,
> + .nlattr.nla_len = 24,
> + .nlattr.nla_type = TEST_POLICY_ATTR,
> + .data = { 0x00, 0x00, 0x00, 0x67, 0xfe, 0x80, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0xe4, 0x1d, 0x2d, 0x03, 0x00, 0xa5, 0xf0, 0x2f
> },
> + .data_len = 20,
> };
> - uint8_t fixture_nl_data_invalid[] = {
> + struct nlattr_fixture fixture_nl_data_invalid = {
> /* valid policy but data neither eth_addr nor ib_addr */
> - 0x0b, 0x00, 0x2a, 0x00, 0x00, 0x53, 0x00, 0x00, 0x00, 0x2a, 0x00,
> + .nlattr.nla_len = 11,
> + .nlattr.nla_type = TEST_POLICY_ATTR,
> + .data = { 0x00, 0x53, 0x00, 0x00, 0x00, 0x2a, 0x00 },
> + .data_len = 7,
> };
> struct ofpbuf *buf;
>
> /* confirm policy fails with too short data */
> buf = ofpbuf_clone_data(&fixture_nl_data_policy_short,
> - sizeof(fixture_nl_data_policy_short));
> + NLA_HDRLEN +
> + fixture_nl_data_policy_short.data_len);
> ovs_assert(!nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> ofpbuf_delete(buf);
> - memset(&attrs, 0, sizeof(*attrs));
> + memset(&attrs, 0, sizeof *attrs);
>
> /* confirm policy fails with too long data */
> buf = ofpbuf_clone_data(&fixture_nl_data_policy_long,
> - sizeof(fixture_nl_data_policy_long));
> + NLA_HDRLEN +
> + fixture_nl_data_policy_long.data_len);
> ovs_assert(!nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> ofpbuf_delete(buf);
> - memset(&attrs, 0, sizeof(*attrs));
> + memset(&attrs, 0, sizeof *attrs);
>
> /* confirm policy passes and interpret valid ethernet lladdr */
> buf = ofpbuf_clone_data(&fixture_nl_data_eth,
> - sizeof(fixture_nl_data_eth));
> + NLA_HDRLEN + fixture_nl_data_eth.data_len);
> ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> ovs_assert((_nla_len(attrs[42]) == sizeof(struct eth_addr)));
> struct eth_addr eth_expect = ETH_ADDR_C(00,53,00,00,00,2a);
> struct eth_addr eth_parsed = nl_attr_get_eth_addr(attrs[42]);
> ovs_assert((!memcmp(ð_expect, ð_parsed, sizeof(struct eth_addr))));
> ofpbuf_delete(buf);
> - memset(&attrs, 0, sizeof(*attrs));
> + memset(&attrs, 0, sizeof *attrs);
>
> /* confirm policy passes and interpret valid infiniband lladdr */
> buf = ofpbuf_clone_data(&fixture_nl_data_ib,
> - sizeof(fixture_nl_data_ib));
> + NLA_HDRLEN + fixture_nl_data_ib.data_len);
> ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> ovs_assert((_nla_len(attrs[42]) == sizeof(struct ib_addr)));
> struct ib_addr ib_expect = {
> @@ -103,18 +128,18 @@ test_nl_policy_parse_ll_addr(struct ovs_cmdl_context
> *ctx OVS_UNUSED) {
> struct ib_addr ib_parsed = nl_attr_get_ib_addr(attrs[42]);
> ovs_assert((!memcmp(&ib_expect, &ib_parsed, sizeof(struct eth_addr))));
> ofpbuf_delete(buf);
> - memset(&attrs, 0, sizeof(*attrs));
> + memset(&attrs, 0, sizeof *attrs);
>
> /* confirm we're able to detect invalid data that passes policy check,
> this
> * can happen because the policy defines the data to be between the
> * currently known lladdr sizes of 6 (ETH_ALEN) and 20 (INFINIBAND_ALEN)
> */
> buf = ofpbuf_clone_data(&fixture_nl_data_invalid,
> - sizeof(fixture_nl_data_invalid));
> + NLA_HDRLEN + fixture_nl_data_invalid.data_len);
> ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy)));
> ovs_assert(_nla_len(attrs[42]) != sizeof(struct eth_addr)
> && _nla_len(attrs[42]) != sizeof(struct ib_addr));
> ofpbuf_delete(buf);
> - memset(&attrs, 0, sizeof(*attrs));
> + memset(&attrs, 0, sizeof *attrs);
> }
>
> static const struct ovs_cmdl_command commands[] = {
> --
> 2.32.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
