netdev - Re: [patch net-next v4 2/7] switchdev: allow caller to explicitly request attr

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]

Date:	Mon, 12 Oct 2015 19:52:42 -0700
From:	Scott Feldman <sfeldma@...il.com>
To:	Jiri Pirko <jiri@...nulli.us>
Cc:	Netdev <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Ido Schimmel <idosch@...lanox.com>,
	Elad Raz <eladr@...lanox.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Guenter Roeck <linux@...ck-us.net>,
	Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
	"andrew@...n.ch" <andrew@...n.ch>,
	john fastabend <john.fastabend@...il.com>,
	David Laight <David.Laight@...lab.com>,
	"stephen@...workplumber.org" <stephen@...workplumber.org>
Subject: Re: [patch net-next v4 2/7] switchdev: allow caller to explicitly
 request attr_set as deferred

On Mon, Oct 12, 2015 at 11:03 AM, Jiri Pirko <jiri@...nulli.us> wrote:
> From: Jiri Pirko <jiri@...lanox.com>
>
> Caller should know if he can call attr_set directly (when holding RTNL)
> or if he has to defer the att_set processing for later.
>
> This also allows drivers to sleep inside attr_set and report operation
> status back to switchdev core. Switchdev core then warns if status is
> not ok, instead of silent errors happening in drivers.
>
> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
> ---
>  include/net/switchdev.h   |   1 +
>  net/bridge/br_stp.c       |   3 +-
>  net/switchdev/switchdev.c | 107 ++++++++++++++++++++++++----------------------
>  3 files changed, 59 insertions(+), 52 deletions(-)
>
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index d2879f2..6b109e4 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -17,6 +17,7 @@
>
>  #define SWITCHDEV_F_NO_RECURSE         BIT(0)
>  #define SWITCHDEV_F_SKIP_EOPNOTSUPP    BIT(1)
> +#define SWITCHDEV_F_DEFER              BIT(2)
>
>  struct switchdev_trans_item {
>         struct list_head list;
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index db6d243de..80c34d7 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -41,13 +41,14 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
>  {
>         struct switchdev_attr attr = {
>                 .id = SWITCHDEV_ATTR_ID_PORT_STP_STATE,
> +               .flags = SWITCHDEV_F_DEFER,
>                 .u.stp_state = state,
>         };
>         int err;
>
>         p->state = state;
>         err = switchdev_port_attr_set(p->dev, &attr);
> -       if (err && err != -EOPNOTSUPP)
> +       if (err)

This looks like a problem as now all other non-switchdev ports will
get an WARN in the log when STP state changes.  We should only WARN if
there was an err and the err is not -EOPNOTSUPP.

>                 br_warn(p->br, "error setting offload STP state on port %u(%s)\n",
>                                 (unsigned int) p->port_no, p->dev->name);
>  }

<snip>

>  struct switchdev_attr_set_work {
>         struct work_struct work;
>         struct net_device *dev;
> @@ -183,14 +226,17 @@ static void switchdev_port_attr_set_work(struct work_struct *work)
>  {
>         struct switchdev_attr_set_work *asw =
>                 container_of(work, struct switchdev_attr_set_work, work);
> +       bool rtnl_locked = rtnl_is_locked();
>         int err;
>
> -       rtnl_lock();
> -       err = switchdev_port_attr_set(asw->dev, &asw->attr);
> +       if (!rtnl_locked)
> +               rtnl_lock();

I'm not following this change.  If someone else has rtnl_lock, we'll
not wait to grab it here ourselves, and proceed as if we have the
lock.  But what if that someone else releases the lock in the middle
of us doing switchdev_port_attr_set_now?  Seems we want to
unconditionally wait and grab the lock.  We need to block anything
from moving while we do the attr set.

> +       err = switchdev_port_attr_set_now(asw->dev, &asw->attr);
>         if (err && err != -EOPNOTSUPP)
>                 netdev_err(asw->dev, "failed (err=%d) to set attribute (id=%d)\n",
>                            err, asw->attr.id);
> -       rtnl_unlock();
> +       if (!rtnl_locked)
> +               rtnl_unlock();
>
>         dev_put(asw->dev);
>         kfree(work);
> @@ -211,7 +257,7 @@ static int switchdev_port_attr_set_defer(struct net_device *dev,
>         asw->dev = dev;
>         memcpy(&asw->attr, attr, sizeof(asw->attr));
>
> -       schedule_work(&asw->work);
> +       queue_work(switchdev_wq, &asw->work);
>
>         return 0;
>  }
--
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