netdev - Re: [PATCH net-next v4 05/24] rocker: use swdev get/set attr for bridge port flags

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]

Message-ID: <20150415052542.GA5891@vergenet.net>
Date:	Wed, 15 Apr 2015 14:25:44 +0900
From:	Simon Horman <simon.horman@...ronome.com>
To:	sfeldma@...il.com
Cc:	netdev@...r.kernel.org, jiri@...nulli.us,
	roopa@...ulusnetworks.com, linux@...ck-us.net,
	f.fainelli@...il.com, sridhar.samudrala@...el.com,
	ronen.arad@...el.com, andrew@...n.ch
Subject: Re: [PATCH net-next v4 05/24] rocker: use swdev get/set attr for
 bridge port flags

Hi Scott,

On Sun, Apr 12, 2015 at 11:16:59PM -0700, sfeldma@...il.com wrote:
> From: Scott Feldman <sfeldma@...il.com>
> 
> Signed-off-by: Scott Feldman <sfeldma@...il.com>
> ---
>  drivers/net/ethernet/rocker/rocker.c |   31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
> index 80c7f6f..9bd4fc6 100644
> --- a/drivers/net/ethernet/rocker/rocker.c
> +++ b/drivers/net/ethernet/rocker/rocker.c

[snip]

> @@ -4349,6 +4354,23 @@ static void rocker_port_trans_abort(struct rocker_port *rocker_port)
>  	}
>  }
>  
> +static int rocker_port_brport_flags_set(struct rocker_port *rocker_port,
> +					unsigned long brport_flags)
> +{
> +	unsigned long orig_flags;
> +	int err = 0;
> +
> +	orig_flags = rocker_port->brport_flags;
> +	rocker_port->brport_flags = brport_flags;
> +	if ((orig_flags ^ rocker_port->brport_flags) & BR_LEARNING)
> +		err = rocker_port_set_learning(rocker_port);

My understanding is that the two-phase set scheme that this
patch-set proposes allows failure, e.g. due resource constraints,
during the prepare phase but that failure in the commit phase
indicates a hardware or driver bug.

It seems to me that this patch doesn't follow the above scheme because
I see the following call-chain:

	rocker_port_set_learning()
	-> rocker_cmd_exec()
	   -> rocker_wait_create()
	      -> kmalloc()

I am probably missing something but it does seem to me that the above is
called in both the prepare and commit phases and that kalloc() may fail in
ether case, the latter case being the problem I see.

> +
> +	if (rocker_port->trans == SWDEV_TRANS_PREPARE)
> +		rocker_port->brport_flags = orig_flags;
> +
> +	return err;
> +}
> +
>  static int rocker_port_attr_set(struct net_device *dev, struct swdev_attr *attr)
>  {
>  	struct rocker_port *rocker_port = netdev_priv(dev);
> @@ -4363,12 +4385,18 @@ static int rocker_port_attr_set(struct net_device *dev, struct swdev_attr *attr)
>  	case SWDEV_TRANS_ABORT:
>  		rocker_port_trans_abort(rocker_port);
>  		return 0;
> +	default:
> +		break;
>  	}
>  
>  	switch (attr->id) {
>  	case SWDEV_ATTR_PORT_STP_STATE:
>  		err = rocker_port_stp_update(rocker_port, attr->stp_state);
>  		break;
> +	case SWDEV_ATTR_PORT_BRIDGE_FLAGS:
> +		err = rocker_port_brport_flags_set(rocker_port,
> +						   attr->brport_flags);
> +		break;
>  	default:
>  		err = -EOPNOTSUPP;
>  		break;
> @@ -4772,8 +4800,7 @@ static int rocker_probe_port(struct rocker *rocker, unsigned int port_number)

[snip]
--
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