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]
Message-ID: <20130410091657.GA27777@verge.net.au>
Date:	Wed, 10 Apr 2013 18:16:57 +0900
From:	Simon Horman <horms@...ge.net.au>
To:	Jesse Gross <jesse@...ira.com>
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 Tue, Apr 09, 2013 at 08:44:02AM -0700, Jesse Gross wrote:
> On Tue, Apr 9, 2013 at 12:50 AM, Simon Horman <horms@...ge.net.au> wrote:
> > On Mon, Apr 08, 2013 at 06:46:29PM -0700, Jesse Gross wrote:
> >> 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.
> >
> > I'm not so sure.
> >
> > I see something like this occurring:
> >
> > 1. Iteration of for loop for output action
> >
> >    switch (nla_type(a)) {
> >    case OVS_ACTION_ATTR_OUTPUT:
> >         prev_port = nla_get_u32(a);
> >         break;
> >         ...
> >    }
> >
> > 2. Iteration of of for loop for next action, lets say its is recirculate
> >
> >    i. Output packet
> >
> >    if (prev_port != -1) {
> >         do_output(dp, skb_clone(skb, GFP_ATOMIC), prev_port);
> >         prev_port = -1;
> >    }
> >
> >    ii. Return due to recirculate
> >    switch (nla_type(a)) {
> >    ...
> >    case OVS_ACTION_ATTR_RECIRCULATE:
> >            return 1;
> >    }
> >
> >
> > Am I missing something?
> 
> Sorry, you're right.
> 
> >> > @@ -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.
> >
> > Sure, perhaps something like this?
> >
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index ab39dd7..721a52c 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -914,7 +914,7 @@ static int validate_and_copy_actions__(const struct nlattr *attr,
> >                         break;
> >
> >                 case OVS_ACTION_ATTR_RECIRCULATE:
> > -                       break;
> > +                       goto out;
> >
> >                 default:
> >                         return -EINVAL;
> > @@ -926,6 +926,7 @@ static int validate_and_copy_actions__(const struct nlattr *attr,
> >                 }
> >         }
> >
> > +out:
> >         if (rem > 0)
> >                 return -EINVAL;
> 
> Since this function is now both validating and copying I think this
> will result in the recirculate action not being copied.

Thanks, I'll look into that.

> >> > 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.
> >
> > I am a little concerned about that too.
> > Perhaps Ben could look over it?
> 
> To rephrase, there are problems in both of those areas. Validation in
> particular I don't think handles resubmitted facets and I believe that
> stats on rules will be the sum of all resubmitted passes.

Some questions:
By resubmitted do you mean recirculated?
What is the stats behaviour that you would like?

With regards to validation, I assume the area of concern
is around facet_revalidate(). I will look into that.

> Both of these will likely significantly affect the data structures, so
> please look into this before we go further.

Sure. I was not planning to push (much) further until this series
is reviewed properly.

> In general, I'd also like
> to see patches that are standalone without needing follow on patches
> to fix known problems (for example, the recirculation ID patches or
> MPLS GSO) unless there is a good reason.

Thanks, I understand. I'll try and structure my patches accordingly.
--
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