netdev - Re: [PATCH 1/4] Add packet recirculation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 8 Apr 2013 18:46:29 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	Simon Horman <horms@...ge.net.au>
Cc:	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>, Ravi K <rkerur@...il.com>,
	Isaku Yamahata <yamahata@...inux.co.jp>,
	Ben Pfaff <blp@...ira.com>
Subject: Re: [PATCH 1/4] Add packet recirculation

On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms@...ge.net.au> wrote:
> diff --git a/datapath/actions.c b/datapath/actions.c
> index e9634fe..7b0f022 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -617,6 +617,9 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>                 case OVS_ACTION_ATTR_SAMPLE:
>                         err = sample(dp, skb, a);
>                         break;
> +
> +               case OVS_ACTION_ATTR_RECIRCULATE:
> +                       return 1;

I think that if we've had a previous output action with the port
stored in prev_port then this will cause the packet to not actually be
output.

> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index e8be795..ab39dd7 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
>  void ovs_dp_process_received_packet(struct vport *p, struct sk_buff *skb)
[...]
> +               if (IS_ERR_OR_NULL(skb)) {
> +                       break;
> +               } else if (unlikely(!limit--)) {

Should this be a predecrement?

> +                       kfree_skb(skb);

Should we log some kind of rate limited warning here?

> +                       return;

In the first case we use break to exit the loop and here we use
return.  Both should have the same effect so it might be nice to make
them the same.

> @@ -901,6 +913,9 @@ static int validate_and_copy_actions__(const struct nlattr *attr,
>                         skip_copy = true;
>                         break;
>
> +               case OVS_ACTION_ATTR_RECIRCULATE:
> +                       break;

I think we might want to jump out the loop here to better model how
the actions are actually executed.

> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index e4a2f75..31255f6 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
>  dp_netdev_port_input(struct dp_netdev *dp, struct dp_netdev_port *port,
>                       struct ofpbuf *packet)
[...]
> +        } else {
> +            dp->n_missed++;
> +            dp_netdev_output_userspace(dp, packet, DPIF_UC_MISS, &key, NULL);
> +            recirculate = false;
> +        }
> +    } while (recirculate && limit--);

I have the same question about predecrement here.

> @@ -1163,6 +1190,7 @@ dp_netdev_sample(struct dp_netdev *dp,
>      const struct nlattr *subactions = NULL;
>      const struct nlattr *a;
>      size_t left;
> +    uint32_t skb_mark;

I don't think it's right to have a new (and uninitialized) copy of
skb_mark here.  We should have the same one all the way through, like
we do in the kernel.

> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 47830c1..5129da1 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c

I'm still working on more detailed comments for this.  However, I'm
concerned about whether the behavior for revalidation and stats is
correct.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html