Re: [libvirt] [PATCH] nwfilter: fix IP address learning

2018-05-26 Thread John Ferlan 


On 05/18/2018 07:59 AM, Daniel P. Berrangé wrote:
> In a previous commit:
> 
>   commit d4bf8f415074759baf051644559e04fe7f8b
>   Author: Daniel P. Berrangé 
>   Date:   Wed Feb 14 09:43:59 2018 +
> 
> nwfilter: handle missing switch enum cases
> 
> Ensure all enum cases are listed in switch statements, or cast away
> enum type in places where we don't wish to cover all cases.
> 
> Reviewed-by: John Ferlan 
> Signed-off-by: Daniel P. Berrangé 
> 
> we changed a switch in the nwfilter learning thread so that it had
> explict cases for all enum entries. Unfortunately the parameters in the
> method had been declared with incorrect type. The "howDetect" parameter
> does *not* accept "enum howDetect" values, rather it accepts a bitmask
> of "enum howDetect" values, so it should have been an "int" type.
> 
> The caller always passes DETECT_STATIC|DETECT_DHCP, so essentially the
> IP addressing learning was completely broken by the above change, as it
> never matched any switch case, hitting the default leading to EINVAL.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/nwfilter/nwfilter_learnipaddr.c | 13 ++---
>  src/nwfilter/nwfilter_learnipaddr.h |  2 +-
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 

This is a companion patch to:

https://www.redhat.com/archives/libvir-list/2018-May/msg01484.html

Still, perhaps to avoid future issues the howDetect related code should
adopt a bitmask model...

Since  Daniel is away and it'd be good to get this into this release
(before he returns) I'll propose an alternative...

John


> diff --git a/src/nwfilter/nwfilter_learnipaddr.c 
> b/src/nwfilter/nwfilter_learnipaddr.c
> index cc3bfd971c..061b39d72b 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.c
> +++ b/src/nwfilter/nwfilter_learnipaddr.c
> @@ -144,7 +144,7 @@ struct _virNWFilterIPAddrLearnReq {
>  char *filtername;
>  virHashTablePtr filterparams;
>  virNWFilterDriverStatePtr driver;
> -enum howDetect howDetect;
> +int howDetect; /* bitmask of enum howDetect */
>  
>  int status;
>  volatile bool terminate;
> @@ -442,23 +442,22 @@ learnIPAddressThread(void *arg)
>  if (techdriver->applyDHCPOnlyRules(req->ifname,
> >macaddr,
> NULL, false) < 0) {
> +VIR_DEBUG("Unable to apply DHCP only rules");
>  req->status = EINVAL;
>  goto done;
>  }
>  virBufferAddLit(, "src port 67 and dst port 68");
>  break;
> -case DETECT_STATIC:
> +default:
>  if (techdriver->applyBasicRules(req->ifname,
>  >macaddr) < 0) {
> +VIR_DEBUG("Unable to apply basic rules");
>  req->status = EINVAL;
>  goto done;
>  }
>  virBufferAsprintf(, "ether host %s or ether dst 
> ff:ff:ff:ff:ff:ff",
>macaddr);
>  break;
> -default:
> -req->status = EINVAL;
> -goto done;
>  }
>  
>  if (virBufferError()) {
> @@ -693,7 +692,7 @@ learnIPAddressThread(void *arg)
>   *   once its IP address has been detected
>   * @driver : the network filter driver
>   * @howDetect : the method on how the thread is supposed to detect the
> - *  IP address; must choose any of the available flags
> + *  IP address; bitmask of "enum howDetect" flags.
>   *
>   * Instruct to learn the IP address being used on a given interface (ifname).
>   * Unless there already is a thread attempting to learn the IP address
> @@ -711,7 +710,7 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr 
> techdriver,
>const char *filtername,
>virHashTablePtr filterparams,
>virNWFilterDriverStatePtr driver,
> -  enum howDetect howDetect)
> +  int howDetect)
>  {
>  int rc;
>  virThread thread;
> diff --git a/src/nwfilter/nwfilter_learnipaddr.h 
> b/src/nwfilter/nwfilter_learnipaddr.h
> index 06fea5bff8..753aabc594 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.h
> +++ b/src/nwfilter/nwfilter_learnipaddr.h
> @@ -43,7 +43,7 @@ int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr 
> techdriver,
>const char *filtername,
>virHashTablePtr filterparams,
>virNWFilterDriverStatePtr driver,
> -  enum howDetect howDetect);
> +  int howDetect);
>  
>  bool virNWFilterHasLearnReq(int ifindex);
>  int virNWFilterTerminateLearnReq(const char *ifname);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] nwfilter: fix IP address learning

2018-05-18 Thread Daniel P . Berrangé 
On Fri, May 18, 2018 at 12:59:01PM +0100, Daniel P. Berrangé wrote:
> In a previous commit:
> 
>   commit d4bf8f415074759baf051644559e04fe7f8b
>   Author: Daniel P. Berrangé 
>   Date:   Wed Feb 14 09:43:59 2018 +
> 
> nwfilter: handle missing switch enum cases
> 
> Ensure all enum cases are listed in switch statements, or cast away
> enum type in places where we don't wish to cover all cases.
> 
> Reviewed-by: John Ferlan 
> Signed-off-by: Daniel P. Berrangé 
> 
> we changed a switch in the nwfilter learning thread so that it had
> explict cases for all enum entries. Unfortunately the parameters in the
> method had been declared with incorrect type. The "howDetect" parameter
> does *not* accept "enum howDetect" values, rather it accepts a bitmask
> of "enum howDetect" values, so it should have been an "int" type.
> 
> The caller always passes DETECT_STATIC|DETECT_DHCP, so essentially the
> IP addressing learning was completely broken by the above change, as it
> never matched any switch case, hitting the default leading to EINVAL.

Sigh, after applying this fix I find on Fedora 28 at least, nwfilter
will always deadlock on vm shutdown

  https://www.redhat.com/archives/libvir-list/2018-May/msg01429.html

I'm guessing we didn't notice this before because Fedora had disabled
TPACKET_V3 in libpcap nutil F28 :-(

So I'm torn about whether to push this or not - broken learning
code for everyone, vs deadlock for anyone with TPACKET_V3
enabled in libpcap (any release since about 2015 seems affected).

> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/nwfilter/nwfilter_learnipaddr.c | 13 ++---
>  src/nwfilter/nwfilter_learnipaddr.h |  2 +-
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/src/nwfilter/nwfilter_learnipaddr.c 
> b/src/nwfilter/nwfilter_learnipaddr.c
> index cc3bfd971c..061b39d72b 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.c
> +++ b/src/nwfilter/nwfilter_learnipaddr.c
> @@ -144,7 +144,7 @@ struct _virNWFilterIPAddrLearnReq {
>  char *filtername;
>  virHashTablePtr filterparams;
>  virNWFilterDriverStatePtr driver;
> -enum howDetect howDetect;
> +int howDetect; /* bitmask of enum howDetect */
>  
>  int status;
>  volatile bool terminate;
> @@ -442,23 +442,22 @@ learnIPAddressThread(void *arg)
>  if (techdriver->applyDHCPOnlyRules(req->ifname,
> >macaddr,
> NULL, false) < 0) {
> +VIR_DEBUG("Unable to apply DHCP only rules");
>  req->status = EINVAL;
>  goto done;
>  }
>  virBufferAddLit(, "src port 67 and dst port 68");
>  break;
> -case DETECT_STATIC:
> +default:
>  if (techdriver->applyBasicRules(req->ifname,
>  >macaddr) < 0) {
> +VIR_DEBUG("Unable to apply basic rules");
>  req->status = EINVAL;
>  goto done;
>  }
>  virBufferAsprintf(, "ether host %s or ether dst 
> ff:ff:ff:ff:ff:ff",
>macaddr);
>  break;
> -default:
> -req->status = EINVAL;
> -goto done;
>  }
>  
>  if (virBufferError()) {
> @@ -693,7 +692,7 @@ learnIPAddressThread(void *arg)
>   *   once its IP address has been detected
>   * @driver : the network filter driver
>   * @howDetect : the method on how the thread is supposed to detect the
> - *  IP address; must choose any of the available flags
> + *  IP address; bitmask of "enum howDetect" flags.
>   *
>   * Instruct to learn the IP address being used on a given interface (ifname).
>   * Unless there already is a thread attempting to learn the IP address
> @@ -711,7 +710,7 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr 
> techdriver,
>const char *filtername,
>virHashTablePtr filterparams,
>virNWFilterDriverStatePtr driver,
> -  enum howDetect howDetect)
> +  int howDetect)
>  {
>  int rc;
>  virThread thread;
> diff --git a/src/nwfilter/nwfilter_learnipaddr.h 
> b/src/nwfilter/nwfilter_learnipaddr.h
> index 06fea5bff8..753aabc594 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.h
> +++ b/src/nwfilter/nwfilter_learnipaddr.h
> @@ -43,7 +43,7 @@ int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr 
> techdriver,
>const char *filtername,
>virHashTablePtr filterparams,
>virNWFilterDriverStatePtr driver,
> -  enum howDetect howDetect);
> +  int howDetect);
>  
>  bool virNWFilterHasLearnReq(int ifindex);
>  int virNWFilterTerminateLearnReq(const char

[libvirt] [PATCH] nwfilter: fix IP address learning

2018-05-18 Thread Daniel P . Berrangé 
In a previous commit:

  commit d4bf8f415074759baf051644559e04fe7f8b
  Author: Daniel P. Berrangé 
  Date:   Wed Feb 14 09:43:59 2018 +

nwfilter: handle missing switch enum cases

Ensure all enum cases are listed in switch statements, or cast away
enum type in places where we don't wish to cover all cases.

Reviewed-by: John Ferlan 
Signed-off-by: Daniel P. Berrangé 

we changed a switch in the nwfilter learning thread so that it had
explict cases for all enum entries. Unfortunately the parameters in the
method had been declared with incorrect type. The "howDetect" parameter
does *not* accept "enum howDetect" values, rather it accepts a bitmask
of "enum howDetect" values, so it should have been an "int" type.

The caller always passes DETECT_STATIC|DETECT_DHCP, so essentially the
IP addressing learning was completely broken by the above change, as it
never matched any switch case, hitting the default leading to EINVAL.

Signed-off-by: Daniel P. Berrangé 
---
 src/nwfilter/nwfilter_learnipaddr.c | 13 ++---
 src/nwfilter/nwfilter_learnipaddr.h |  2 +-
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/src/nwfilter/nwfilter_learnipaddr.c 
b/src/nwfilter/nwfilter_learnipaddr.c
index cc3bfd971c..061b39d72b 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -144,7 +144,7 @@ struct _virNWFilterIPAddrLearnReq {
 char *filtername;
 virHashTablePtr filterparams;
 virNWFilterDriverStatePtr driver;
-enum howDetect howDetect;
+int howDetect; /* bitmask of enum howDetect */
 
 int status;
 volatile bool terminate;
@@ -442,23 +442,22 @@ learnIPAddressThread(void *arg)
 if (techdriver->applyDHCPOnlyRules(req->ifname,
>macaddr,
NULL, false) < 0) {
+VIR_DEBUG("Unable to apply DHCP only rules");
 req->status = EINVAL;
 goto done;
 }
 virBufferAddLit(, "src port 67 and dst port 68");
 break;
-case DETECT_STATIC:
+default:
 if (techdriver->applyBasicRules(req->ifname,
 >macaddr) < 0) {
+VIR_DEBUG("Unable to apply basic rules");
 req->status = EINVAL;
 goto done;
 }
 virBufferAsprintf(, "ether host %s or ether dst ff:ff:ff:ff:ff:ff",
   macaddr);
 break;
-default:
-req->status = EINVAL;
-goto done;
 }
 
 if (virBufferError()) {
@@ -693,7 +692,7 @@ learnIPAddressThread(void *arg)
  *   once its IP address has been detected
  * @driver : the network filter driver
  * @howDetect : the method on how the thread is supposed to detect the
- *  IP address; must choose any of the available flags
+ *  IP address; bitmask of "enum howDetect" flags.
  *
  * Instruct to learn the IP address being used on a given interface (ifname).
  * Unless there already is a thread attempting to learn the IP address
@@ -711,7 +710,7 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr 
techdriver,
   const char *filtername,
   virHashTablePtr filterparams,
   virNWFilterDriverStatePtr driver,
-  enum howDetect howDetect)
+  int howDetect)
 {
 int rc;
 virThread thread;
diff --git a/src/nwfilter/nwfilter_learnipaddr.h 
b/src/nwfilter/nwfilter_learnipaddr.h
index 06fea5bff8..753aabc594 100644
--- a/src/nwfilter/nwfilter_learnipaddr.h
+++ b/src/nwfilter/nwfilter_learnipaddr.h
@@ -43,7 +43,7 @@ int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr 
techdriver,
   const char *filtername,
   virHashTablePtr filterparams,
   virNWFilterDriverStatePtr driver,
-  enum howDetect howDetect);
+  int howDetect);
 
 bool virNWFilterHasLearnReq(int ifindex);
 int virNWFilterTerminateLearnReq(const char *ifname);
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list