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

Daniel P. Berrangé berrange at redhat.com
Fri May 18 17:29:33 UTC 2018


On Fri, May 18, 2018 at 12:59:01PM +0100, Daniel P. Berrangé wrote:
> In a previous commit:
> 
>   commit d4bf8f415074759baf051644559e04fe78888f8b
>   Author: Daniel P. Berrangé <berrange at redhat.com>
>   Date:   Wed Feb 14 09:43:59 2018 +0000
> 
>     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 <jferlan at redhat.com>
>     Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> 
> 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é <berrange at redhat.com>
> ---
>  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,
>                                             &req->macaddr,
>                                             NULL, false) < 0) {
> +            VIR_DEBUG("Unable to apply DHCP only rules");
>              req->status = EINVAL;
>              goto done;
>          }
>          virBufferAddLit(&buf, "src port 67 and dst port 68");
>          break;
> -    case DETECT_STATIC:
> +    default:
>          if (techdriver->applyBasicRules(req->ifname,
>                                          &req->macaddr) < 0) {
> +            VIR_DEBUG("Unable to apply basic rules");
>              req->status = EINVAL;
>              goto done;
>          }
>          virBufferAsprintf(&buf, "ether host %s or ether dst ff:ff:ff:ff:ff:ff",
>                            macaddr);
>          break;
> -    default:
> -        req->status = EINVAL;
> -        goto done;
>      }
>  
>      if (virBufferError(&buf)) {
> @@ -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
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|




More information about the libvir-list mailing list