[ovs-dev] [PATCH] netdev-linux: Fix self-deadlocks in traffic control code.

[Andy Zhou]

Looks good.
Acked-by: Andy Zhou <azhou at nicira.com>


On Fri, Aug 16, 2013 at 1:09 PM, Ben Pfaff <blp at nicira.com> wrote:

> htb_parse_qdisc_details__(), which is called with the netdev mutex, called
> netdev_get_mtu(), which tried to reacquire the mutex and thus deadlocked.
> This commit fixes the problem and similar problems in
> htb_parse_class_details__() and hfsc_parse_qdisc_details__().
>
> Bug #19180.
> Reported-by: Dhaval Badiani <dbadiani at vmware.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/netdev-linux.c |   41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 9a80b67..c3684da 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -1041,21 +1041,17 @@ netdev_linux_get_etheraddr(const struct netdev
> *netdev_,
>      return error;
>  }
>
> -/* Returns the maximum size of transmitted (and received) packets on
> 'netdev',
> - * in bytes, not including the hardware header; thus, this is typically
> 1500
> - * bytes for Ethernet devices. */
>  static int
> -netdev_linux_get_mtu(const struct netdev *netdev_, int *mtup)
> +netdev_linux_get_mtu__(struct netdev_linux *netdev, int *mtup)
> +    OVS_REQUIRES(netdev->mutex)
>  {
> -    struct netdev_linux *netdev = netdev_linux_cast(netdev_);
>      int error;
>
> -    ovs_mutex_lock(&netdev->mutex);
>      if (!(netdev->cache_valid & VALID_MTU)) {
>          struct ifreq ifr;
>
>          netdev->netdev_mtu_error = af_inet_ifreq_ioctl(
> -            netdev_get_name(netdev_), &ifr, SIOCGIFMTU, "SIOCGIFMTU");
> +            netdev_get_name(&netdev->up), &ifr, SIOCGIFMTU, "SIOCGIFMTU");
>          netdev->mtu = ifr.ifr_mtu;
>          netdev->cache_valid |= VALID_MTU;
>      }
> @@ -1064,6 +1060,21 @@ netdev_linux_get_mtu(const struct netdev *netdev_,
> int *mtup)
>      if (!error) {
>          *mtup = netdev->mtu;
>      }
> +
> +    return error;
> +}
> +
> +/* Returns the maximum size of transmitted (and received) packets on
> 'netdev',
> + * in bytes, not including the hardware header; thus, this is typically
> 1500
> + * bytes for Ethernet devices. */
> +static int
> +netdev_linux_get_mtu(const struct netdev *netdev_, int *mtup)
> +{
> +    struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> +    int error;
> +
> +    ovs_mutex_lock(&netdev->mutex);
> +    error = netdev_linux_get_mtu__(netdev, mtup);
>      ovs_mutex_unlock(&netdev->mutex);
>
>      return error;
> @@ -2707,7 +2718,7 @@ htb_setup_class__(struct netdev *netdev, unsigned
> int handle,
>      int error;
>      int mtu;
>
> -    error = netdev_get_mtu(netdev, &mtu);
> +    error = netdev_linux_get_mtu__(netdev_linux_cast(netdev), &mtu);
>      if (error) {
>          VLOG_WARN_RL(&rl, "cannot set up HTB on device %s that lacks MTU",
>                       netdev_get_name(netdev));
> @@ -2803,9 +2814,10 @@ htb_parse_tcmsg__(struct ofpbuf *tcmsg, unsigned
> int *queue_id,
>  }
>
>  static void
> -htb_parse_qdisc_details__(struct netdev *netdev,
> +htb_parse_qdisc_details__(struct netdev *netdev_,
>                            const struct smap *details, struct htb_class
> *hc)
>  {
> +    struct netdev_linux *netdev = netdev_linux_cast(netdev_);
>      const char *max_rate_s;
>
>      max_rate_s = smap_get(details, "max-rate");
> @@ -2813,7 +2825,8 @@ htb_parse_qdisc_details__(struct netdev *netdev,
>      if (!hc->max_rate) {
>          enum netdev_features current;
>
> -        netdev_get_features(netdev, &current, NULL, NULL, NULL);
> +        netdev_linux_read_features(netdev);
> +        current = !netdev->get_features_error ? netdev->current : 0;
>          hc->max_rate = netdev_features_to_bps(current, 100 * 1000 * 1000)
> / 8;
>      }
>      hc->min_rate = hc->max_rate;
> @@ -2832,7 +2845,7 @@ htb_parse_class_details__(struct netdev *netdev,
>      const char *priority_s = smap_get(details, "priority");
>      int mtu, error;
>
> -    error = netdev_get_mtu(netdev, &mtu);
> +    error = netdev_linux_get_mtu__(netdev_linux_cast(netdev), &mtu);
>      if (error) {
>          VLOG_WARN_RL(&rl, "cannot parse HTB class on device %s that lacks
> MTU",
>                       netdev_get_name(netdev));
> @@ -3280,9 +3293,10 @@ hfsc_query_class__(const struct netdev *netdev,
> unsigned int handle,
>  }
>
>  static void
> -hfsc_parse_qdisc_details__(struct netdev *netdev, const struct smap
> *details,
> +hfsc_parse_qdisc_details__(struct netdev *netdev_, const struct smap
> *details,
>                             struct hfsc_class *class)
>  {
> +    struct netdev_linux *netdev = netdev_linux_cast(netdev_);
>      uint32_t max_rate;
>      const char *max_rate_s;
>
> @@ -3292,7 +3306,8 @@ hfsc_parse_qdisc_details__(struct netdev *netdev,
> const struct smap *details,
>      if (!max_rate) {
>          enum netdev_features current;
>
> -        netdev_get_features(netdev, &current, NULL, NULL, NULL);
> +        netdev_linux_read_features(netdev);
> +        current = !netdev->get_features_error ? netdev->current : 0;
>          max_rate = netdev_features_to_bps(current, 100 * 1000 * 1000) / 8;
>      }
>
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130816/6d8b987a/attachment-0003.html>