Re: [PATCH] datapath: Fix an error handling path in 'ovs_nla_init_match_and_action()'

[Greg Rose]

On 09/11/2017 12:20 PM, Christophe JAILLET wrote:

All other error handling paths in this function go through the 'error'
label. This one should do the same.

Fixes: 9cc9a5cb176c ("datapath: Avoid using stack larger than 1024.")
Signed-off-by: Christophe JAILLET 
---
I think that the comment above the function could be improved. It looks
like the commit log which has introduced this function.

I'm also not sure that commit 9cc9a5cb176c is of any help. It is
supposed to remove a warning, and I guess it does. But 
'ovs_nla_init_match_and_action()'
is called unconditionnaly from 'ovs_flow_cmd_set()'. So even if the stack
used by each function is reduced, the overall stack should be the same, if
not larger.

So this commit sounds like adding a bug where the code was fine and states
to fix an issue but, at the best, only hides it.


Having a large stack frame isn't really a bug per se.  But the Linux kernel
warns about stack frames that are too large so reordering the code to
get the warning to go away seems fine to me.



Instead of fixing the code with the proposed patch, reverting the initial
commit could also be considered.


Then the warning will come back.

- Greg


---
  net/openvswitch/datapath.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 76cf273a56c7..c3aec6227c91 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1112,7 +1112,8 @@ static int ovs_nla_init_match_and_action(struct net *net,
if (!a[OVS_FLOW_ATTR_KEY]) {
OVS_NLERR(log,
  "Flow key attribute not present in set 
flow.");
-   return -EINVAL;
+   error = -EINVAL;
+   goto error;
}
  
  		*acts = get_flow_actions(net, a[OVS_FLOW_ATTR_ACTIONS], key,

[PATCH V3 net] openvswitch: Fix for force/commit action failures

[Greg Rose]

When there is an established connection in direction A->B, it is
possible to receive a packet on port B which then executes
ct(commit,force) without first performing ct() - ie, a lookup.
In this case, we would expect that this packet can delete the existing
entry so that we can commit a connection with direction B->A. However,
currently we only perform a check in skb_nfct_cached() for whether
OVS_CS_F_TRACKED is set and OVS_CS_F_INVALID is not set, ie that a
lookup previously occurred. In the above scenario, a lookup has not
occurred but we should still be able to statelessly look up the
existing entry and potentially delete the entry if it is in the
opposite direction.

This patch extends the check to also hint that if the action has the
force flag set, then we will lookup the existing entry so that the
force check at the end of skb_nfct_cached has the ability to delete
the connection.

Fixes: dd41d330b03 ("openvswitch: Add force commit.")
CC: Pravin Shelar <pshe...@nicira.com>
CC: d...@openvswitch.org
Signed-off-by: Joe Stringer <j...@ovn.org>
Signed-off-by: Greg Rose <gvrose8...@gmail.com>
---

V2: Make sure nf_conntrack_in() is called for force case
V3: Fix compiler warning for newer compilers
---
 net/openvswitch/conntrack.c | 51 -
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 08679eb..e3c4c6c 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -629,6 +629,34 @@ static int handle_fragments(struct net *net, struct 
sw_flow_key *key,
return ct;
 }
 
+static
+struct nf_conn *ovs_ct_executed(struct net *net,
+   const struct sw_flow_key *key,
+   const struct ovs_conntrack_info *info,
+   struct sk_buff *skb,
+   bool *ct_executed)
+{
+   struct nf_conn *ct = NULL;
+
+   /* If no ct, check if we have evidence that an existing conntrack entry
+* might be found for this skb.  This happens when we lose a skb->_nfct
+* due to an upcall, or if the direction is being forced.  If the
+* connection was not confirmed, it is not cached and needs to be run
+* through conntrack again.
+*/
+   *ct_executed = (key->ct_state & OVS_CS_F_TRACKED) &&
+  !(key->ct_state & OVS_CS_F_INVALID) &&
+  (key->ct_zone == info->zone.id);
+
+   if (*ct_executed || (!key->ct_state && info->force)) {
+   ct = ovs_ct_find_existing(net, >zone, info->family, skb,
+ !!(key->ct_state &
+ OVS_CS_F_NAT_MASK));
+   }
+
+   return ct;
+}
+
 /* Determine whether skb->_nfct is equal to the result of conntrack lookup. */
 static bool skb_nfct_cached(struct net *net,
const struct sw_flow_key *key,
@@ -637,24 +665,17 @@ static bool skb_nfct_cached(struct net *net,
 {
enum ip_conntrack_info ctinfo;
struct nf_conn *ct;
+   bool ct_executed = true;
 
ct = nf_ct_get(skb, );
-   /* If no ct, check if we have evidence that an existing conntrack entry
-* might be found for this skb.  This happens when we lose a skb->_nfct
-* due to an upcall.  If the connection was not confirmed, it is not
-* cached and needs to be run through conntrack again.
-*/
-   if (!ct && key->ct_state & OVS_CS_F_TRACKED &&
-   !(key->ct_state & OVS_CS_F_INVALID) &&
-   key->ct_zone == info->zone.id) {
-   ct = ovs_ct_find_existing(net, >zone, info->family, skb,
- !!(key->ct_state
-& OVS_CS_F_NAT_MASK));
-   if (ct)
-   nf_ct_get(skb, );
-   }
if (!ct)
+   ct = ovs_ct_executed(net, key, info, skb, _executed);
+
+   if (ct)
+   nf_ct_get(skb, );
+   else
return false;
+
if (!net_eq(net, read_pnet(>ct_net)))
return false;
if (!nf_ct_zone_equal_any(info->ct, nf_ct_zone(ct)))
@@ -679,7 +700,7 @@ static bool skb_nfct_cached(struct net *net,
return false;
}
 
-   return true;
+   return ct_executed;
 }
 
 #ifdef CONFIG_NF_NAT_NEEDED
-- 
1.8.3.1

Re: [PATCH net] openvswitch: Fix for force/commit action failures

[Greg Rose]

On 07/14/2017 11:42 AM, Joe Stringer wrote:

On 14 July 2017 at 09:10, Greg Rose <gvrose8...@gmail.com> wrote:
> When there is an established connection in direction A->B, it is
> possible to receive a packet on port B which then executes
> ct(commit,force) without first performing ct() - ie, a lookup.
> In this case, we would expect that this packet can delete the existing
> entry so that we can commit a connection with direction B->A. However,
> currently we only perform a check in skb_nfct_cached() for whether
> OVS_CS_F_TRACKED is set and OVS_CS_F_INVALID is not set, ie that a
> lookup previously occurred. In the above scenario, a lookup has not
> occurred but we should still be able to statelessly look up the
> existing entry and potentially delete the entry if it is in the
> opposite direction.
>
> This patch extends the check to also hint that if the action has the
> force flag set, then we will lookup the existing entry so that the
> force check at the end of skb_nfct_cached has the ability to delete
> the connection.
>
> Fixes: dd41d330b03 ("openvswitch: Add force commit.")
> CC: Pravin Shelar <pshe...@nicira.com>
> CC: d...@openvswitch.org
> Signed-off-by: Joe Stringer <j...@ovn.org>
> Signed-off-by: Greg Rose <gvrose8...@gmail.com>
> ---
>   net/openvswitch/conntrack.c | 50 
+++--
>   1 file changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 08679eb..1260f2b 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -629,6 +629,33 @@ static int handle_fragments(struct net *net, struct 
sw_flow_key *key,
>  return ct;
>   }
>
> +struct nf_conn *ovs_ct_executed(struct net *net,
> +   const struct sw_flow_key *key,
> +   const struct ovs_conntrack_info *info,
> +   struct sk_buff *skb,
> +   bool *ct_executed)

Actually, some compilers will report this new warning:
net/openvswitch/conntrack.c:632:16: warning: symbol 'ovs_ct_executed'
was not declared. Should it be static?

Could you make this function static and repost?

Thanks.


Yes, I just saw that too.  Need to update my compiler for my primary build 
machine.  I'll send a V2...

Thanks!

- Greg

[PATCH net] openvswitch: Fix for force/commit action failures

[Greg Rose]

When there is an established connection in direction A->B, it is
possible to receive a packet on port B which then executes
ct(commit,force) without first performing ct() - ie, a lookup.
In this case, we would expect that this packet can delete the existing
entry so that we can commit a connection with direction B->A. However,
currently we only perform a check in skb_nfct_cached() for whether
OVS_CS_F_TRACKED is set and OVS_CS_F_INVALID is not set, ie that a
lookup previously occurred. In the above scenario, a lookup has not
occurred but we should still be able to statelessly look up the
existing entry and potentially delete the entry if it is in the
opposite direction.

This patch extends the check to also hint that if the action has the
force flag set, then we will lookup the existing entry so that the
force check at the end of skb_nfct_cached has the ability to delete
the connection.

Fixes: dd41d330b03 ("openvswitch: Add force commit.")
CC: Pravin Shelar <pshe...@nicira.com>
CC: d...@openvswitch.org
Signed-off-by: Joe Stringer <j...@ovn.org>
Signed-off-by: Greg Rose <gvrose8...@gmail.com>
---
 net/openvswitch/conntrack.c | 50 +++--
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 08679eb..1260f2b 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -629,6 +629,33 @@ static int handle_fragments(struct net *net, struct 
sw_flow_key *key,
return ct;
 }
 
+struct nf_conn *ovs_ct_executed(struct net *net,
+   const struct sw_flow_key *key,
+   const struct ovs_conntrack_info *info,
+   struct sk_buff *skb,
+   bool *ct_executed)
+{
+   struct nf_conn *ct = NULL;
+
+   /* If no ct, check if we have evidence that an existing conntrack entry
+* might be found for this skb.  This happens when we lose a skb->_nfct
+* due to an upcall, or if the direction is being forced.  If the
+* connection was not confirmed, it is not cached and needs to be run
+* through conntrack again.
+*/
+   *ct_executed = (key->ct_state & OVS_CS_F_TRACKED) &&
+  !(key->ct_state & OVS_CS_F_INVALID) &&
+  (key->ct_zone == info->zone.id);
+
+   if (*ct_executed || (!key->ct_state && info->force)) {
+   ct = ovs_ct_find_existing(net, >zone, info->family, skb,
+ !!(key->ct_state &
+ OVS_CS_F_NAT_MASK));
+   }
+
+   return ct;
+}
+
 /* Determine whether skb->_nfct is equal to the result of conntrack lookup. */
 static bool skb_nfct_cached(struct net *net,
const struct sw_flow_key *key,
@@ -637,24 +664,17 @@ static bool skb_nfct_cached(struct net *net,
 {
enum ip_conntrack_info ctinfo;
struct nf_conn *ct;
+   bool ct_executed = true;
 
ct = nf_ct_get(skb, );
-   /* If no ct, check if we have evidence that an existing conntrack entry
-* might be found for this skb.  This happens when we lose a skb->_nfct
-* due to an upcall.  If the connection was not confirmed, it is not
-* cached and needs to be run through conntrack again.
-*/
-   if (!ct && key->ct_state & OVS_CS_F_TRACKED &&
-   !(key->ct_state & OVS_CS_F_INVALID) &&
-   key->ct_zone == info->zone.id) {
-   ct = ovs_ct_find_existing(net, >zone, info->family, skb,
- !!(key->ct_state
-& OVS_CS_F_NAT_MASK));
-   if (ct)
-   nf_ct_get(skb, );
-   }
if (!ct)
+   ct = ovs_ct_executed(net, key, info, skb, _executed);
+
+   if (ct)
+   nf_ct_get(skb, );
+   else
return false;
+
if (!net_eq(net, read_pnet(>ct_net)))
return false;
if (!nf_ct_zone_equal_any(info->ct, nf_ct_zone(ct)))
@@ -679,7 +699,7 @@ static bool skb_nfct_cached(struct net *net,
return false;
}
 
-   return true;
+   return ct_executed;
 }
 
 #ifdef CONFIG_NF_NAT_NEEDED
-- 
1.8.3.1

Re: [PATCH] datapath: Fix for force/commit action failures

[Greg Rose]

On 07/13/2017 11:03 AM, Joe Stringer wrote:

On 13 July 2017 at 11:01, Greg Rose <gvrose8...@gmail.com> wrote:

On 07/13/2017 10:46 AM, Joe Stringer wrote:


On 13 July 2017 at 09:25, Greg Rose <gvrose8...@gmail.com> wrote:


When there is an established connection in direction A->B, it is
possible to receive a packet on port B which then executes
ct(commit,force) without first performing ct() - ie, a lookup.
In this case, we would expect that this packet can delete the existing
entry so that we can commit a connection with direction B->A. However,
currently we only perform a check in skb_nfct_cached() for whether
OVS_CS_F_TRACKED is set and OVS_CS_F_INVALID is not set, ie that a
lookup previously occurred. In the above scenario, a lookup has not
occurred but we should still be able to statelessly look up the
existing entry and potentially delete the entry if it is in the
opposite direction.

This patch extends the check to also hint that if the action has the
force flag set, then we will lookup the existing entry so that the
force check at the end of skb_nfct_cached has the ability to delete
the connection.

CC: d...@openvswitch.org
CC: Pravin Shalar <pshe...@nicira.com>
Signed-off-by: Joe Stringer <j...@ovn.org>
Signed-off-by: Greg Rose <gvrose8...@gmail.com>
---



A couple more administrative notes, on netdev the module name in the
patch subject for openvswitch is "openvswitch" rather than datapath;



Right you are.


and patches rather than having just "PATCH" as the subject prefix
should state the tree. In this case, it's a bugfix so it should be
"PATCH net".



I knew that... forgot the format patch option to add it.  Net-next
is closed so that would be mandatory.

  Furthermore, if you're able to figure out which commit


introduced the issue (I believe it's introduced by the force commit
patch), then you should place the "Fixes: " tag. I can give you some
pointers off-list on how to do this (git blame and some basic
formatting of the targeted patch should do the trick - this tag
expects a 12-digit hash).

For reference, I ended up looking it up during review, this is the
line you'd add:
Fixes: dd41d33f0b03 ("openvswitch: Add force commit.")



Oh, thanks!





   net/openvswitch/conntrack.c | 12 
   1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 08679eb..9041cf8 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -641,17 +641,21 @@ static bool skb_nfct_cached(struct net *net,
  ct = nf_ct_get(skb, );
  /* If no ct, check if we have evidence that an existing
conntrack entry
   * might be found for this skb.  This happens when we lose a
skb->_nfct
-* due to an upcall.  If the connection was not confirmed, it is
not
-* cached and needs to be run through conntrack again.
+* due to an upcall, or if the direction is being forced.  If the
+* connection was not confirmed, it is not cached and needs to be
run
+* through conntrack again.
   */
-   if (!ct && key->ct_state & OVS_CS_F_TRACKED &&
+   if ((!ct && (key->ct_state & OVS_CS_F_TRACKED) &&
  !(key->ct_state & OVS_CS_F_INVALID) &&
-   key->ct_zone == info->zone.id) {
+key->ct_zone == info->zone.id) ||
+(!key->ct_state && info->force)) {
  ct = ovs_ct_find_existing(net, >zone,
info->family, skb,
!!(key->ct_state
   & OVS_CS_F_NAT_MASK));
  if (ct)
  nf_ct_get(skb, );
+   else
+   return false;
  }
  if (!ct)
  return false;



I was just wondering if this has the potential to prevent
nf_conntrack_in() from being called at all in this case, which is also
not quite right. In the original case of (!ct && (key->ct_state &
OVS_CS_F_TRACKED) && !(key->ct_state & OVS_CS_F_TRACKED)), which I'll
refer to as "ct_executed", we explicitly want to avoid running
nf_conntrack_in() if we already ran it, because the connection tracker
doesn't expect to see the same packet twice (there's also things like
stats/accounting, and potentially L4 state machines that could get
messed up by multiple calls). By the time the info->force and
direction check happens at the end of the function, "ct_executed" is
implied to be true. However, in this new case, ct_executed is actually
false - because there was no ct() before the ct(force,commit). In this
case, we only want to look up the existing entry to see if it should
be deleted; if it should not be deleted, then we still haven't yet
done the nf_

Re: [PATCH] datapath: Fix for force/commit action failures

[Greg Rose]

On 07/13/2017 10:46 AM, Joe Stringer wrote:

On 13 July 2017 at 09:25, Greg Rose <gvrose8...@gmail.com> wrote:

When there is an established connection in direction A->B, it is
possible to receive a packet on port B which then executes
ct(commit,force) without first performing ct() - ie, a lookup.
In this case, we would expect that this packet can delete the existing
entry so that we can commit a connection with direction B->A. However,
currently we only perform a check in skb_nfct_cached() for whether
OVS_CS_F_TRACKED is set and OVS_CS_F_INVALID is not set, ie that a
lookup previously occurred. In the above scenario, a lookup has not
occurred but we should still be able to statelessly look up the
existing entry and potentially delete the entry if it is in the
opposite direction.

This patch extends the check to also hint that if the action has the
force flag set, then we will lookup the existing entry so that the
force check at the end of skb_nfct_cached has the ability to delete
the connection.

CC: d...@openvswitch.org
CC: Pravin Shalar <pshe...@nicira.com>
Signed-off-by: Joe Stringer <j...@ovn.org>
Signed-off-by: Greg Rose <gvrose8...@gmail.com>
---


A couple more administrative notes, on netdev the module name in the
patch subject for openvswitch is "openvswitch" rather than datapath;


Right you are.


and patches rather than having just "PATCH" as the subject prefix
should state the tree. In this case, it's a bugfix so it should be
"PATCH net".


I knew that... forgot the format patch option to add it.  Net-next
is closed so that would be mandatory.

 Furthermore, if you're able to figure out which commit

introduced the issue (I believe it's introduced by the force commit
patch), then you should place the "Fixes: " tag. I can give you some
pointers off-list on how to do this (git blame and some basic
formatting of the targeted patch should do the trick - this tag
expects a 12-digit hash).

For reference, I ended up looking it up during review, this is the
line you'd add:
Fixes: dd41d33f0b03 ("openvswitch: Add force commit.")


Oh, thanks!




  net/openvswitch/conntrack.c | 12 
  1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 08679eb..9041cf8 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -641,17 +641,21 @@ static bool skb_nfct_cached(struct net *net,
 ct = nf_ct_get(skb, );
 /* If no ct, check if we have evidence that an existing conntrack entry
  * might be found for this skb.  This happens when we lose a skb->_nfct
-* due to an upcall.  If the connection was not confirmed, it is not
-* cached and needs to be run through conntrack again.
+* due to an upcall, or if the direction is being forced.  If the
+* connection was not confirmed, it is not cached and needs to be run
+* through conntrack again.
  */
-   if (!ct && key->ct_state & OVS_CS_F_TRACKED &&
+   if ((!ct && (key->ct_state & OVS_CS_F_TRACKED) &&
 !(key->ct_state & OVS_CS_F_INVALID) &&
-   key->ct_zone == info->zone.id) {
+key->ct_zone == info->zone.id) ||
+(!key->ct_state && info->force)) {
 ct = ovs_ct_find_existing(net, >zone, info->family, skb,
   !!(key->ct_state
  & OVS_CS_F_NAT_MASK));
 if (ct)
 nf_ct_get(skb, );
+   else
+   return false;
 }
 if (!ct)
 return false;


I was just wondering if this has the potential to prevent
nf_conntrack_in() from being called at all in this case, which is also
not quite right. In the original case of (!ct && (key->ct_state &
OVS_CS_F_TRACKED) && !(key->ct_state & OVS_CS_F_TRACKED)), which I'll
refer to as "ct_executed", we explicitly want to avoid running
nf_conntrack_in() if we already ran it, because the connection tracker
doesn't expect to see the same packet twice (there's also things like
stats/accounting, and potentially L4 state machines that could get
messed up by multiple calls). By the time the info->force and
direction check happens at the end of the function, "ct_executed" is
implied to be true. However, in this new case, ct_executed is actually
false - because there was no ct() before the ct(force,commit). In this
case, we only want to look up the existing entry to see if it should
be deleted; if it should not be deleted, then we still haven't yet
done the nf_conntrack_in() call so we should return false and the
caller, __ovs_ct_lookup() should make the call to nf_conntrack_in().

What I mean is something

Re: [ovs-dev] [PATCH] datapath: Fix for force/commit action failures

[Greg Rose]

On 07/13/2017 10:08 AM, Darrell Ball wrote:



On 7/13/17, 9:25 AM, "ovs-dev-boun...@openvswitch.org on behalf of Greg Rose" 
<ovs-dev-boun...@openvswitch.org on behalf of gvrose8...@gmail.com> wrote:

 When there is an established connection in direction A->B, it is
 possible to receive a packet on port B which then executes
 ct(commit,force) without first performing ct() - ie, a lookup.
 In this case, we would expect that this packet can delete the existing
 entry so that we can commit a connection with direction B->A. However,
 currently we only perform a check in skb_nfct_cached() for whether
 OVS_CS_F_TRACKED is set and OVS_CS_F_INVALID is not set, ie that a
 lookup previously occurred. In the above scenario, a lookup has not
 occurred but we should still be able to statelessly look up the
 existing entry and potentially delete the entry if it is in the
 opposite direction.
 
 This patch extends the check to also hint that if the action has the

 force flag set, then we will lookup the existing entry so that the
 force check at the end of skb_nfct_cached has the ability to delete
 the connection.
 
 CC: d...@openvswitch.org

 CC: Pravin Shalar <pshe...@nicira.com>
 Signed-off-by: Joe Stringer <j...@ovn.org>
 Signed-off-by: Greg Rose <gvrose8...@gmail.com>
 ---
  net/openvswitch/conntrack.c | 12 
  1 file changed, 8 insertions(+), 4 deletions(-)
 
 diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c

 index 08679eb..9041cf8 100644
 --- a/net/openvswitch/conntrack.c
 +++ b/net/openvswitch/conntrack.c
 @@ -641,17 +641,21 @@ static bool skb_nfct_cached(struct net *net,
ct = nf_ct_get(skb, );
/* If no ct, check if we have evidence that an existing conntrack entry
 * might be found for this skb.  This happens when we lose a skb->_nfct
 -   * due to an upcall.  If the connection was not confirmed, it is not
 -   * cached and needs to be run through conntrack again.
 +   * due to an upcall, or if the direction is being forced.  If the
 +   * connection was not confirmed, it is not cached and needs to be run
 +   * through conntrack again.
 */
 -  if (!ct && key->ct_state & OVS_CS_F_TRACKED &&
 +  if ((!ct && (key->ct_state & OVS_CS_F_TRACKED) &&
!(key->ct_state & OVS_CS_F_INVALID) &&
 -  key->ct_zone == info->zone.id) {
 +   key->ct_zone == info->zone.id) ||
 +   (!key->ct_state && info->force)) {
ct = ovs_ct_find_existing(net, >zone, info->family, skb,
  !!(key->ct_state
 & OVS_CS_F_NAT_MASK));
if (ct)
nf_ct_get(skb, );
 +  else
 +  return false;

the above else case looks redundant since it maps to the following check
if (!ct)
return false;
which services a superset of the code flow.


Sure, we can let it fall through... missed that.

After waiting for more review I'll send a V2 patch.

Thanks for the review Darrell

- Greg



}
if (!ct)
return false;
 --
 1.8.3.1
 
 ___

 dev mailing list
 d...@openvswitch.org
 
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=5da6ykiSQJBoJXmpq6iVknmPAc5JDzlVngmp8j_xcXA=PsX-njQ_hFqy8P77KNEyX0i7u165p2Wrbg4uAYqfbGs=
 

[PATCH] datapath: Fix for force/commit action failures

[Greg Rose]

When there is an established connection in direction A->B, it is
possible to receive a packet on port B which then executes
ct(commit,force) without first performing ct() - ie, a lookup.
In this case, we would expect that this packet can delete the existing
entry so that we can commit a connection with direction B->A. However,
currently we only perform a check in skb_nfct_cached() for whether
OVS_CS_F_TRACKED is set and OVS_CS_F_INVALID is not set, ie that a
lookup previously occurred. In the above scenario, a lookup has not
occurred but we should still be able to statelessly look up the
existing entry and potentially delete the entry if it is in the
opposite direction.

This patch extends the check to also hint that if the action has the
force flag set, then we will lookup the existing entry so that the
force check at the end of skb_nfct_cached has the ability to delete
the connection.

CC: d...@openvswitch.org
CC: Pravin Shalar <pshe...@nicira.com>
Signed-off-by: Joe Stringer <j...@ovn.org>
Signed-off-by: Greg Rose <gvrose8...@gmail.com>
---
 net/openvswitch/conntrack.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 08679eb..9041cf8 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -641,17 +641,21 @@ static bool skb_nfct_cached(struct net *net,
ct = nf_ct_get(skb, );
/* If no ct, check if we have evidence that an existing conntrack entry
 * might be found for this skb.  This happens when we lose a skb->_nfct
-* due to an upcall.  If the connection was not confirmed, it is not
-* cached and needs to be run through conntrack again.
+* due to an upcall, or if the direction is being forced.  If the
+* connection was not confirmed, it is not cached and needs to be run
+* through conntrack again.
 */
-   if (!ct && key->ct_state & OVS_CS_F_TRACKED &&
+   if ((!ct && (key->ct_state & OVS_CS_F_TRACKED) &&
!(key->ct_state & OVS_CS_F_INVALID) &&
-   key->ct_zone == info->zone.id) {
+key->ct_zone == info->zone.id) ||
+(!key->ct_state && info->force)) {
ct = ovs_ct_find_existing(net, >zone, info->family, skb,
  !!(key->ct_state
 & OVS_CS_F_NAT_MASK));
if (ct)
nf_ct_get(skb, );
+   else
+   return false;
}
if (!ct)
return false;
-- 
1.8.3.1

Re: [Intel-wired-lan] [PATCH 1/2] i40e/i40evf: rename vf_offload_flags to vf_cap_flags in struct virtchnl_vf_resource

[Greg Rose]

On Thu, Jul 6, 2017 at 4:26 PM, Wyborny, Carolyn
<carolyn.wybo...@intel.com> wrote:
>> -Original Message-----
>> From: Greg Rose [mailto:gvrose8...@gmail.com]
>> Sent: Thursday, July 06, 2017 7:25 AM
>> To: Wyborny, Carolyn <carolyn.wybo...@intel.com>
>> Cc: Stefan Assmann <sassm...@kpanic.de>; intel-wired-...@lists.osuosl.org;
>> netdev@vger.kernel.org; da...@davemloft.net
>> Subject: Re: [Intel-wired-lan] [PATCH 1/2] i40e/i40evf: rename
>> vf_offload_flags to vf_cap_flags in struct virtchnl_vf_resource
>>
>> On Wed, Jul 5, 2017 at 10:15 PM, Wyborny, Carolyn
>> <carolyn.wybo...@intel.com> wrote:
>> >
>> > > -Original Message-
>> > > From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On
>> Behalf
>> > > Of Stefan Assmann
>> > > Sent: Thursday, June 29, 2017 6:12 AM
>> > > To: intel-wired-...@lists.osuosl.org
>> > > Cc: netdev@vger.kernel.org; da...@davemloft.net;
>> sassm...@kpanic.de
>> > > Subject: [Intel-wired-lan] [PATCH 1/2] i40e/i40evf: rename
>> vf_offload_flags to
>> > > vf_cap_flags in struct virtchnl_vf_resource
>> > >
>> > > The current name of vf_offload_flags indicates that the bitmap is
>> > > limited to offload related features. Make this more generic by renaming
>> > > it to vf_cap_flags, which allows for other capabilities besides
>> > > offloading to be added.
>> > >
>> > > Signed-off-by: Stefan Assmann <sassm...@kpanic.de>
>> > > ---
>> >
>> > Hello Stefan,
>> >
>> > Thanks for the patch series, but we believe the vf should be ignorant of 
>> > its
>> trusted status.
>>
>> Hi Carolyn,
>>
>> Might I ask why?
>>
>> Thanks,
>>
>> - Greg
>
> Hello Greg,
>
> The trusted status of the vf is something that pf manages, mostly for 
> security reasons.  The mailbox model of communication between them relies on 
> the pf to have authority over the vf.  In most things, the vf asks permission 
> from the pf for changes in its configuration and this keeps the pf as the 
> gatekeeper between trusted and regular vf's.  However, the issue here is that 
> changing the MAC address from within the vf does not go through the mailbox, 
> so it cannot tell if the vf is trusted or not.   We are working on a redesign 
> of this feature to fix that instead of having the vf sort of know its 
> trusted.  If, for some reason, we are unable to redesign it that way we would 
> reconsider this method instead.
>
> Thanks,

Thanks, that was my concern, that by allowing the VF to set it's own
MAC then it would implicitly know it was trusted.

Regards,

- Greg
>
> Carolyn

Re: [Intel-wired-lan] [PATCH 1/2] i40e/i40evf: rename vf_offload_flags to vf_cap_flags in struct virtchnl_vf_resource

[Greg Rose]

On Wed, Jul 5, 2017 at 10:15 PM, Wyborny, Carolyn
 wrote:
>
> > -Original Message-
> > From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On Behalf
> > Of Stefan Assmann
> > Sent: Thursday, June 29, 2017 6:12 AM
> > To: intel-wired-...@lists.osuosl.org
> > Cc: netdev@vger.kernel.org; da...@davemloft.net; sassm...@kpanic.de
> > Subject: [Intel-wired-lan] [PATCH 1/2] i40e/i40evf: rename vf_offload_flags 
> > to
> > vf_cap_flags in struct virtchnl_vf_resource
> >
> > The current name of vf_offload_flags indicates that the bitmap is
> > limited to offload related features. Make this more generic by renaming
> > it to vf_cap_flags, which allows for other capabilities besides
> > offloading to be added.
> >
> > Signed-off-by: Stefan Assmann 
> > ---
>
> Hello Stefan,
>
> Thanks for the patch series, but we believe the vf should be ignorant of its 
> trusted status.

Hi Carolyn,

Might I ask why?

Thanks,

- Greg

Re: [PATCH] datapath: Avoid using stack larger than 1024.

[Greg Rose]

On 06/27/2017 12:03 AM, Tonghao Zhang wrote:

When compiling OvS-master on 4.4.0-81 kernel,
there is a warning:

 CC [M]  /root/ovs/datapath/linux/datapath.o
 /root/ovs/datapath/linux/datapath.c: In function
 ‘ovs_flow_cmd_set’:
 /root/ovs/datapath/linux/datapath.c:1221:1: warning:
 the frame size of 1040 bytes is larger than 1024 bytes
 [-Wframe-larger-than=]

This patch use kmalloc to malloc mem for sw_flow_mask and
avoid using stack.

Signed-off-by: Tonghao Zhang 
---
  datapath/datapath.c | 11 ---
  1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index c85029c..da8cd68 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1107,7 +1107,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct 
genl_info *info)
  struct ovs_header *ovs_header = info->userhdr;
  struct sw_flow_key key;
  struct sw_flow *flow;
-struct sw_flow_mask mask;
+struct sw_flow_mask *mask;
  struct sk_buff *reply = NULL;
  struct datapath *dp;
  struct sw_flow_actions *old_acts = NULL, *acts = NULL;
@@ -1120,7 +1120,11 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct 
genl_info *info)

  ufid_present = ovs_nla_get_ufid(, a[OVS_FLOW_ATTR_UFID], log);
  if (a[OVS_FLOW_ATTR_KEY]) {
-ovs_match_init(, , true, );
+mask = kmalloc(sizeof(struct sw_flow_mask), GFP_KERNEL);
+if (!mask)
+return -ENOMEM;
+
+ovs_match_init(, , true, mask);
  error = ovs_nla_get_match(net, , a[OVS_FLOW_ATTR_KEY],
a[OVS_FLOW_ATTR_MASK], log);
  } else if (!ufid_present) {
@@ -1141,7 +1145,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct 
genl_info *info)
  }

  acts = get_flow_actions(net, a[OVS_FLOW_ATTR_ACTIONS], ,
-, log);
+mask, log);
  if (IS_ERR(acts)) {
  error = PTR_ERR(acts);
  goto error;
@@ -1216,6 +1220,7 @@ err_unlock_ovs:
  err_kfree_acts:
  ovs_nla_free_flow_actions(acts);
  error:
+kfree(mask);
  return error;
  }



It looks fine to me but let's copy the maintainer Pravin

- Greg

Re: [PATCH net] net: Zero ifla_vf_info in rtnl_fill_vfinfo()

[Greg Rose]

On 06/07/2017 11:00 AM, Yuval Mintz wrote:

Some of the structure's fields are not initialized by the
rtnetlink. If driver doesn't set those in ndo_get_vf_config(),
they'd leak memory to user.

Signed-off-by: Yuval Mintz <yuval.mi...@cavium.com>
CC: Michal Schmidt <mschm...@redhat.com>
---
  net/core/rtnetlink.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 9e2c0a7..5e61456 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1124,6 +1124,8 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct 
sk_buff *skb,
  struct ifla_vf_mac vf_mac;
  struct ifla_vf_info ivi;

+memset(, 0, sizeof(ivi));
+
  /* Not all SR-IOV capable drivers support the
   * spoofcheck and "RSS query enable" query.  Preset to
   * -1 so the user space tool can detect that the driver
@@ -1132,7 +1134,6 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct 
sk_buff *skb,
  ivi.spoofchk = -1;
  ivi.rss_query_en = -1;
  ivi.trusted = -1;
-memset(ivi.mac, 0, sizeof(ivi.mac));
  /* The default value for VF link state is "auto"
   * IFLA_VF_LINK_STATE_AUTO which equals zero
   */


It's been a few years since I worked in this code but I do recall this portion. 
 Good idea...

Thanks!

Reviewed-by: Greg Rose <gvrose8...@gmail.com>

Re: [PATCH] net: rtnetlink: bail out from rtnl_fdb_dump() on parse error

[Greg Rose]

rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3231,8 +3231,11 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct 
> netlink_callback *cb)
>   int err = 0;
>   int fidx = 0;
>  
> - if (nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> - IFLA_MAX, ifla_policy, NULL) == 0) {
> + err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> +   IFLA_MAX, ifla_policy, NULL);
> + if (err < 0) {
> + return -EINVAL;
> + } else if (err == 0) {
>   if (tb[IFLA_MASTER])
>   br_idx = nla_get_u32(tb[IFLA_MASTER]);
>   }

Fix looks right to me.

Reviewed-by: Greg Rose <gvrose8...@gmail.com>

Re: [PATCH net-next] net: vrf: Do not allow looback to be moved to a VRF

[Greg Rose]

On Wed, 2017-04-26 at 07:58 -0700, David Ahern wrote:
> Moving the loopback into a VRF breaks networking for the default VRF.
> Since the VRF device is the loopback for VRF domains, there is no
> reason to move the loopback. Given the repercussions, block attempts
> to set lo into a VRF.
> 
> Signed-off-by: David Ahern <d...@cumulusnetworks.com>
> ---
>  drivers/net/vrf.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index aa5d30428bba..ceda5861da78 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -877,6 +877,12 @@ static int do_vrf_add_slave(struct net_device *dev, 
> struct net_device *port_dev)
>  {
>   int ret;
>  
> + /* do not allow loopback device to be enslaved to a VRF.
> +  * The vrf device acts as the loopback for the vrf.
> +  */
> + if (port_dev == dev_net(dev)->loopback_dev)
> + return -EOPNOTSUPP;
> +
>   port_dev->priv_flags |= IFF_L3MDEV_SLAVE;
>   ret = netdev_master_upper_dev_link(port_dev, dev, NULL, NULL);
>   if (ret < 0)

I think that's a great idea.

Reviewed-by: Greg Rose <gvrose8...@gmail.com>

Re: [PATCH net] netvsc: fix calculation of available send sections

[Greg Rose]

On Mon, 2017-04-24 at 18:33 -0700, Stephen Hemminger wrote:
> My change (introduced in 4.11) to use find_first_clear_bit
> incorrectly assumed that the size argument was words, not bits.

Oops...

> The effect was only a small limited number of the available send
> sections were being actually used. This can cause performance loss
> with some workloads.
> 
> Since map_words is now used only during initialization, it can
> be on stack instead of in per-device data.
> 
> Fixes: b58a185801da ("netvsc: simplify get next send section")
> Signed-off-by: Stephen Hemminger <sthem...@microsoft.com>

Looks good to me.

Reviewed by Greg Rose <gvrose8...@gmail.com>

> ---
>  drivers/net/hyperv/hyperv_net.h | 1 -
>  drivers/net/hyperv/netvsc.c | 9 -
>  2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index f9f3dba7a588..db23cb36ae5c 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -751,7 +751,6 @@ struct netvsc_device {
>   u32 send_section_cnt;
>   u32 send_section_size;
>   unsigned long *send_section_map;
> - int map_words;
>  
>   /* Used for NetVSP initialization protocol */
>   struct completion channel_init_wait;
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 8dd0b8770328..15ef713d96c0 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -236,6 +236,7 @@ static int netvsc_init_buf(struct hv_device *device)
>   struct netvsc_device *net_device;
>   struct nvsp_message *init_packet;
>   struct net_device *ndev;
> + size_t map_words;
>   int node;
>  
>   net_device = get_outbound_net_device(device);
> @@ -401,11 +402,9 @@ static int netvsc_init_buf(struct hv_device *device)
>  net_device->send_section_size, net_device->send_section_cnt);
>  
>   /* Setup state for managing the send buffer. */
> - net_device->map_words = DIV_ROUND_UP(net_device->send_section_cnt,
> -  BITS_PER_LONG);
> + map_words = DIV_ROUND_UP(net_device->send_section_cnt, BITS_PER_LONG);
>  
> - net_device->send_section_map = kcalloc(net_device->map_words,
> -sizeof(ulong), GFP_KERNEL);
> + net_device->send_section_map = kcalloc(map_words, sizeof(ulong), 
> GFP_KERNEL);
>   if (net_device->send_section_map == NULL) {
>   ret = -ENOMEM;
>   goto cleanup;
> @@ -683,7 +682,7 @@ static u32 netvsc_get_next_send_section(struct 
> netvsc_device *net_device)
>   unsigned long *map_addr = net_device->send_section_map;
>   unsigned int i;
>  
> - for_each_clear_bit(i, map_addr, net_device->map_words) {
> + for_each_clear_bit(i, map_addr, net_device->send_section_cnt) {
>   if (sync_test_and_set_bit(i, map_addr) == 0)
>   return i;
>   }

Re: [PATCH] p54: Prevent from dereferencing null pointer when releasing SKB

[Greg Rose]

On Thu, 2017-04-20 at 16:23 -0700, Myungho Jung wrote:
> On Thu, Apr 20, 2017 at 04:03:43PM -0700, Greg Rose wrote:
> > On Thu, 2017-04-20 at 11:25 -0700, Myungho Jung wrote:
> > > Added NULL check to make __dev_kfree_skb_irq consistent with kfree
> > > family of functions.
> > > 
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289
> > > 
> > > Signed-off-by: Myungho Jung <mhju...@gmail.com>
> > 
> > Hi,
> > 
> > I think the patch is fine but I'm confused by the subject.  You mention
> > p54 driver but the change is in dev.c.  I know the bugzilla references
> > the p54 but that's not where the change is.
> > 
> > Seems odd to me.
> > 
> > - Greg
> > 
> > > ---
> > >  net/core/dev.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 7869ae3..22be2a6 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -2450,6 +2450,9 @@ void __dev_kfree_skb_irq(struct sk_buff *skb, enum 
> > > skb_free_reason reason)
> > >  {
> > >   unsigned long flags;
> > >  
> > > + if (unlikely(!skb))
> > > + return;
> > > +
> > >   if (likely(atomic_read(>users) == 1)) {
> > >   smp_rmb();
> > >   atomic_set(>users, 0);
> > 
> > 
> > 
> 
> Hi Greg,
> 
> Thank you for checking my patch. I missed that I moved change from p54
> to net/dev. Do I need to resubmit patch v2 to modify subject?
> 
> Thanks,
> Myungho

Yes, I think that's the best idea.

Thanks,

- Greg

Re: [PATCH] p54: Prevent from dereferencing null pointer when releasing SKB

[Greg Rose]

On Thu, 2017-04-20 at 11:25 -0700, Myungho Jung wrote:
> Added NULL check to make __dev_kfree_skb_irq consistent with kfree
> family of functions.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289
> 
> Signed-off-by: Myungho Jung 

Hi,

I think the patch is fine but I'm confused by the subject.  You mention
p54 driver but the change is in dev.c.  I know the bugzilla references
the p54 but that's not where the change is.

Seems odd to me.

- Greg

> ---
>  net/core/dev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 7869ae3..22be2a6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2450,6 +2450,9 @@ void __dev_kfree_skb_irq(struct sk_buff *skb, enum 
> skb_free_reason reason)
>  {
>   unsigned long flags;
>  
> + if (unlikely(!skb))
> + return;
> +
>   if (likely(atomic_read(>users) == 1)) {
>   smp_rmb();
>   atomic_set(>users, 0);

Re: [PATCH net-next 2/2] openvswitch: Add eventmask support to CT action.

[Greg Rose]

On Wed, 2017-04-19 at 18:55 -0700, Jarno Rajahalme wrote:
> Add a new optional conntrack action attribute OVS_CT_ATTR_EVENTMASK,
> which can be used in conjunction with the commit flag
> (OVS_CT_ATTR_COMMIT) to set the mask of bits specifying which
> conntrack events (IPCT_*) should be delivered via the Netfilter
> netlink multicast groups.  Default behavior depends on the system
> configuration, but typically a lot of events are delivered.  This can be
> very chatty for the NFNLGRP_CONNTRACK_UPDATE group, even if only some
> types of events are of interest.
> 
> Netfilter core init_conntrack() adds the event cache extension, so we
> only need to set the ctmask value.  However, if the system is
> configured without support for events, the setting will be skipped due
> to extension not being found.
> 
> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>

Looks good to me but I'll let Joe ack it since I'm not that familiar
with the code yet.  Nevertheless...

Reviewed-by: Greg Rose <gvrose@8...@gmail.com>

> ---
>  include/uapi/linux/openvswitch.h | 12 
>  net/openvswitch/conntrack.c  | 27 +++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/include/uapi/linux/openvswitch.h 
> b/include/uapi/linux/openvswitch.h
> index 66d1c3c..38ae95d 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -693,6 +693,17 @@ struct ovs_action_hash {
>   * nothing if the connection is already committed will check that the current
>   * packet is in conntrack entry's original direction.  If directionality does
>   * not match, will delete the existing conntrack entry and commit a new one.
> + * @OVS_CT_ATTR_EVENTMASK: Mask of bits indicating which conntrack event 
> types
> + * (enum ip_conntrack_events IPCT_*) should be reported.  For any bit set to
> + * zero, the corresponding event type is not generated.  Default behavior
> + * depends on system configuration, but typically all event types are
> + * generated, hence listening on UPDATE events may get a lot of events.
> + * Explicitly passing this attribute allows limiting the updates received to
> + * the events of interest.  The bit 1 << IPCT_NEW, 1 << IPCT_RELATED, and
> + * 1 << IPCT_DESTROY must be set to ones for those events to be received on
> + * NFNLGRP_CONNTRACK_NEW and NFNLGRP_CONNTRACK_DESTROY groups, respectively.
> + * Remaining bits control the changes for which an event is delivered on the
> + * NFNLGRP_CONNTRACK_UPDATE group.
>   */
>  enum ovs_ct_attr {
>   OVS_CT_ATTR_UNSPEC,
> @@ -704,6 +715,7 @@ enum ovs_ct_attr {
>  related connections. */
>   OVS_CT_ATTR_NAT,/* Nested OVS_NAT_ATTR_* */
>   OVS_CT_ATTR_FORCE_COMMIT,  /* No argument */
> + OVS_CT_ATTR_EVENTMASK,  /* u32 mask of IPCT_* events. */
>   __OVS_CT_ATTR_MAX
>  };
>  
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 58de4c2..4f7c3b5 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -66,7 +66,9 @@ struct ovs_conntrack_info {
>   u8 commit : 1;
>   u8 nat : 3; /* enum ovs_ct_nat */
>   u8 force : 1;
> + u8 have_eventmask : 1;
>   u16 family;
> + u32 eventmask;  /* Mask of 1 << IPCT_*. */
>   struct md_mark mark;
>   struct md_labels labels;
>  #ifdef CONFIG_NF_NAT_NEEDED
> @@ -1007,6 +1009,20 @@ static int ovs_ct_commit(struct net *net, struct 
> sw_flow_key *key,
>   if (!ct)
>   return 0;
>  
> + /* Set the conntrack event mask if given.  NEW and DELETE events have
> +  * their own groups, but the NFNLGRP_CONNTRACK_UPDATE group listener
> +  * typically would receive many kinds of updates.  Setting the event
> +  * mask allows those events to be filtered.  The set event mask will
> +  * remain in effect for the lifetime of the connection unless changed
> +  * by a further CT action with both the commit flag and the eventmask
> +  * option. */
> + if (info->have_eventmask) {
> + struct nf_conntrack_ecache *cache = nf_ct_ecache_find(ct);
> +
> + if (cache)
> + cache->ctmask = info->eventmask;
> + }
> +
>   /* Apply changes before confirming the connection so that the initial
>* conntrack NEW netlink event carries the values given in the CT
>* action.
> @@ -1238,6 +1254,8 @@ static const struct ovs_ct_len_tbl 
> ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
>   /* NAT length is checked when parsing the nested attributes. */
>   [OVS_CT_ATTR_NAT]   = { .minlen = 0, .maxlen = INT_MAX },
>  #endi

Re: [PATCH net-next 1/2] openvswitch: Typo fix.

[Greg Rose]

On Wed, 2017-04-19 at 18:55 -0700, Jarno Rajahalme wrote:
> Fix typo in a comment.
> 
> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
> ---
>  net/openvswitch/conntrack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 7b2c2fc..58de4c2 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -373,7 +373,7 @@ static int ovs_ct_init_labels(struct nf_conn *ct, struct 
> sw_flow_key *key,
>   }
>  
>   /* Labels are included in the IPCTNL_MSG_CT_NEW event only if the
> -  * IPCT_LABEL bit it set in the event cache.
> +  * IPCT_LABEL bit is set in the event cache.
>*/
>   nf_conntrack_event_cache(IPCT_LABEL, ct);
>  

Acked-by: Greg Rose <gvrose8...@gmail.com>

Re: vlan tagging problem

[Greg Rose]

On Tue, 2017-04-11 at 15:36 -0400, carl h wrote:
> Hi
> 
> Hope this is right place to ask this. Looking for help regarding vlan
> tagging done by the 8021q driver.
> 
> I tried to manually configure a vlan on my target device. I use these
> 2 vlan config commands, then I ping an IP address and sniff the
> packets to look for the vlan tag in the messages.
> 
>vconfig add esw0
>ifconfig esw0.10 10.93.1.2 netmask 255.255.255.0
> 
> Despite my attempts, tagging does not occur in any outgoing packets.
> However when I tried the exact same method and commands on my Ubuntu
> workstation vlan tagging did work without problems.
> 
> Target: mips32, Linux 2.6.39, 802.1Q driver version 1.8
> 
> I have looked at the source for the 8021q.ko driver and I keep running
> into a dead end trying to figure out where the 4 bytes of vlan data
> gets added to the IP header. There are 2 key functions where vlan
> information is deciphered and added to a structure that contains vlan
> info, but the information never gets added to the header for some
> reason.
> 
>vlan_dev_hard_header()
>vlan_dev_hard_start_xmit()
> 
> It is clear in the code where vlan information in receive packets gets
> removed, but not so clear in the transmit direction where this is
> supposed to happen.
> 
> Could someone explain to me what I'm missing, and where and how vlan
> info gets added to the header for outgoing packets?
> 
> thx
> /carl

The Ethernet HW probably inserts the VLAN tag for you as an 'offload'.

Run 'ethtool -k ' and check the offloads.  You can change the
offload settings with 'ethtool -K..'.

- Greg Rose

[PATCH] net/ethernet: Use ether_addr_copy rather than memcpy

[Greg Rose]

I'm not sure why this hasn't been done before because it seems obvious,
so maybe there is some reason that memcpy is used instead of
ether_addr_copy in this code.  But let's try this anyway.

Change memcpy to ether_addr_copy.

Signed-off-by: Greg Rose <gr...@lightfleet.com>
---
 net/ethernet/eth.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 66dff5e..cbcdb97 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -95,10 +95,10 @@ int eth_header(struct sk_buff *skb, struct net_device *dev,
 
if (!saddr)
saddr = dev->dev_addr;
-   memcpy(eth->h_source, saddr, ETH_ALEN);
+   ether_addr_copy(eth->h_source, saddr);
 
if (daddr) {
-   memcpy(eth->h_dest, daddr, ETH_ALEN);
+   ether_addr_copy(eth->h_dest, daddr);
return ETH_HLEN;
}
 
@@ -211,7 +211,7 @@ EXPORT_SYMBOL(eth_type_trans);
 int eth_header_parse(const struct sk_buff *skb, unsigned char *haddr)
 {
const struct ethhdr *eth = eth_hdr(skb);
-   memcpy(haddr, eth->h_source, ETH_ALEN);
+   ether_addr_copy(haddr, eth->h_source);
return ETH_ALEN;
 }
 EXPORT_SYMBOL(eth_header_parse);
@@ -236,8 +236,8 @@ int eth_header_cache(const struct neighbour *neigh, struct 
hh_cache *hh, __be16
return -1;
 
eth->h_proto = type;
-   memcpy(eth->h_source, dev->dev_addr, ETH_ALEN);
-   memcpy(eth->h_dest, neigh->ha, ETH_ALEN);
+   ether_addr_copy(eth->h_source, dev->dev_addr);
+   ether_addr_copy(eth->h_dest, neigh->ha);
hh->hh_len = ETH_HLEN;
return 0;
 }
@@ -255,8 +255,8 @@ void eth_header_cache_update(struct hh_cache *hh,
 const struct net_device *dev,
 const unsigned char *haddr)
 {
-   memcpy(((u8 *) hh->hh_data) + HH_DATA_OFF(sizeof(struct ethhdr)),
-  haddr, ETH_ALEN);
+   ether_addr_copy(((u8 *)hh->hh_data) +
+   HH_DATA_OFF(sizeof(struct ethhdr)), haddr);
 }
 EXPORT_SYMBOL(eth_header_cache_update);
 
@@ -286,7 +286,7 @@ void eth_commit_mac_addr_change(struct net_device *dev, 
void *p)
 {
struct sockaddr *addr = p;
 
-   memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
+   ether_addr_copy(dev->dev_addr, addr->sa_data);
 }
 EXPORT_SYMBOL(eth_commit_mac_addr_change);
 
-- 
1.8.3.1

Re: [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Greg Rose]

On Wed, Jun 1, 2016 at 3:07 PM, Phil Sutter <p...@nwl.cc> wrote:
> On Wed, Jun 01, 2016 at 03:00:08PM -0700, Greg Rose wrote:
>> On Wed, Jun 1, 2016 at 1:03 PM, Phil Sutter <p...@nwl.cc> wrote:
>> > Not sure whether I misinterpret commit 7b8179c780a1a, but it looks
>> > overly complicated. Instead rely upon parse_rtattr_nested() to assign
>> > the relevant pointer if requested rtattr fields are present.
>>
>> I'm not sure if newer iproute2 utilities are supposed to work on older
>> kernels but if it is you may want to check this against a 2.6.32
>> kernel.
>
> Yes, it is supposed to. Actually I tried, but the old RHEL6 kernel I
> used didn't export the VF list at all and then I lost motivation.
>
> I didn't check all earlier versions of 7b8179c780a1a, was there a stage
> when it looked like what I'm changing it to?

I don't think so but your patch looks correct - I mean it looks like
it should work.

It's been 5 years since I wrote that original patch and my memory
isn't so great as to why I didn't just do as your patch does but I
think it had something to do with not all drivers reporting a spoof
check value.  However, your patch should handle that case so I see no
reason not to accept it.  Unfortunately I don't have time or the
resources at the moment to check it on an older kernel.

- Greg

>
> Cheers, Phil

Re: [iproute PATCH 1/2] ipaddress: Simplify vf_info parsing

[Greg Rose]

On Wed, Jun 1, 2016 at 1:03 PM, Phil Sutter <p...@nwl.cc> wrote:
> Not sure whether I misinterpret commit 7b8179c780a1a, but it looks
> overly complicated. Instead rely upon parse_rtattr_nested() to assign
> the relevant pointer if requested rtattr fields are present.

I'm not sure if newer iproute2 utilities are supposed to work on older
kernels but if it is you may want to check this against a 2.6.32
kernel.

- Greg Rose

>
> Signed-off-by: Phil Sutter <p...@nwl.cc>
> ---
>  ip/ipaddress.c | 44 ++--
>  1 file changed, 10 insertions(+), 34 deletions(-)
>
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index df363b070d5de..9ac077459f136 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -305,10 +305,7 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
> struct ifla_vf_mac *vf_mac;
> struct ifla_vf_vlan *vf_vlan;
> struct ifla_vf_tx_rate *vf_tx_rate;
> -   struct ifla_vf_spoofchk *vf_spoofchk;
> -   struct ifla_vf_link_state *vf_linkstate;
> struct rtattr *vf[IFLA_VF_MAX + 1] = {};
> -   struct rtattr *tmp;
>
> SPRINT_BUF(b1);
>
> @@ -323,31 +320,6 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
> vf_vlan = RTA_DATA(vf[IFLA_VF_VLAN]);
> vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
>
> -   /* Check if the spoof checking vf info type is supported by
> -* this kernel.
> -*/
> -   tmp = (struct rtattr *)((char *)vf[IFLA_VF_TX_RATE] +
> -   vf[IFLA_VF_TX_RATE]->rta_len);
> -
> -   if (tmp->rta_type != IFLA_VF_SPOOFCHK)
> -   vf_spoofchk = NULL;
> -   else
> -   vf_spoofchk = RTA_DATA(vf[IFLA_VF_SPOOFCHK]);
> -
> -   if (vf_spoofchk) {
> -   /* Check if the link state vf info type is supported by
> -* this kernel.
> -*/
> -   tmp = (struct rtattr *)((char *)vf[IFLA_VF_SPOOFCHK] +
> -   vf[IFLA_VF_SPOOFCHK]->rta_len);
> -
> -   if (tmp->rta_type != IFLA_VF_LINK_STATE)
> -   vf_linkstate = NULL;
> -   else
> -   vf_linkstate = RTA_DATA(vf[IFLA_VF_LINK_STATE]);
> -   } else
> -   vf_linkstate = NULL;
> -
> fprintf(fp, "%svf %d MAC %s", _SL_, vf_mac->vf,
> ll_addr_n2a((unsigned char *)_mac->mac,
> ETH_ALEN, 0, b1, sizeof(b1)));
> @@ -366,14 +338,18 @@ static void print_vfinfo(FILE *fp, struct rtattr 
> *vfinfo)
> if (vf_rate->min_tx_rate)
> fprintf(fp, ", min_tx_rate %dMbps", 
> vf_rate->min_tx_rate);
> }
> +   if (vf[IFLA_VF_SPOOFCHK]) {
> +   struct ifla_vf_spoofchk *vf_spoofchk =
> +   RTA_DATA(vf[IFLA_VF_SPOOFCHK]);
>
> -   if (vf_spoofchk && vf_spoofchk->setting != -1) {
> -   if (vf_spoofchk->setting)
> -   fprintf(fp, ", spoof checking on");
> -   else
> -   fprintf(fp, ", spoof checking off");
> +   if (vf_spoofchk->setting != -1)
> +   fprintf(fp, ", spoof checking %s",
> +   vf_spoofchk->setting ? "on" : "off");
> }
> -   if (vf_linkstate) {
> +   if (vf[IFLA_VF_LINK_STATE]) {
> +   struct ifla_vf_link_state *vf_linkstate =
> +   RTA_DATA(vf[IFLA_VF_LINK_STATE]);
> +
> if (vf_linkstate->link_state == IFLA_VF_LINK_STATE_AUTO)
> fprintf(fp, ", link-state auto");
> else if (vf_linkstate->link_state == 
> IFLA_VF_LINK_STATE_ENABLE)
> --
> 2.8.2
>