netdev - Re: [PATCH net] net: dsa: mv88e6xxx: Fix opps when adding vlan bridge

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: <20161212130416.GC27057@lunn.ch>
Date:   Mon, 12 Dec 2016 14:04:16 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Vivien Didelot <vivien.didelot@...oirfairelinux.com>
Cc:     David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: dsa: mv88e6xxx: Fix opps when adding vlan bridge

On Sun, Dec 11, 2016 at 04:02:37PM -0500, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@...n.ch> writes:
> 
> > @@ -1804,6 +1807,9 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
> >  			    chip->ports[port].bridge_dev)
> >  				break; /* same bridge, check next VLAN */
> >  
> > +			if (!chip->ports[i].bridge_dev)
> > +				continue;
> > +
> 
> The above truncated test:
> 
> 			if (chip->ports[i].bridge_dev ==
> 			    chip->ports[port].bridge_dev)
> 				break; /* same bridge, check next VLAN */
> 
> should handle the case where bridge_dev is NULL, but if you want to
> explicitly test it, I'd move it before this statement.
> 
> >  			netdev_warn(ds->ports[port].netdev,
> >  				    "hardware VLAN %d already used by %s\n",
> >  				    vlan.vid,

Hi Vivien

I don't think you comment is correct. Here is the loop, with my two
additions.

                for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
                        if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
                                continue;

                        if (!ds->ports[port].netdev)
                                continue;

                        if (vlan.data[i] ==
                            GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
                                continue;

                        if (chip->ports[i].bridge_dev ==
                            chip->ports[port].bridge_dev)
                                break; /* same bridge, check next VLAN */

                        if (!chip->ports[i].bridge_dev)
                                continue;

                        netdev_warn(ds->ports[port].netdev,
                                    "hardware VLAN %d already used by %s\n",
                                    vlan.vid,
                                    netdev_name(chip->ports[i].bridge_dev));
                        err = -EOPNOTSUPP;
                        goto unlock;
                }

The opps was occurring in netdev_name(). I did not check which
one. The obvious one, or the netdev_warn(netdev,... one.

> The above truncated test:
> 
> 			if (chip->ports[i].bridge_dev ==
> 			    chip->ports[port].bridge_dev)
> 				break; /* same bridge, check next VLAN */
>
> should handle the case where bridge_dev is NULL, but if you want to
> explicitly test it, I'd move it before this statement.

This will not stop chip->ports[i].bridge_dev == NULL from reaching the
netdev_warn().

	Andrew