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
| ||
|
Date: Thu, 12 Oct 2017 21:12:31 +0300 From: Nikolay Aleksandrov <nikolay@...ulusnetworks.com> To: Roman Mashak <mrv@...atatu.com>, David Ahern <dsahern@...il.com> Cc: David Miller <davem@...emloft.net>, Stephen Hemminger <stephen@...workplumber.org>, Linux Kernel Network Developers <netdev@...r.kernel.org> Subject: Re: [PATCH net-next v2 1/1] bridge: return error code when deleting Vlan On 12/10/17 21:07, Roman Mashak wrote: > On Thu, Oct 12, 2017 at 10:19 AM, David Ahern <dsahern@...il.com> wrote: >> On 10/12/17 7:51 AM, Roman Mashak wrote: >>> v2: >>> Return err immediately if nbp_vlan_delete() fails (pointed by David Ahern) >>> >>> Signed-off-by: Roman Mashak <mrv@...atatu.com> >>> --- >>> net/bridge/br_netlink.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >>> index f0e8268..1efdd48 100644 >>> --- a/net/bridge/br_netlink.c >>> +++ b/net/bridge/br_netlink.c >>> @@ -527,11 +527,13 @@ static int br_vlan_info(struct net_bridge *br, struct net_bridge_port *p, >>> >>> case RTM_DELLINK: >>> if (p) { >>> - nbp_vlan_delete(p, vinfo->vid); >>> + err = nbp_vlan_delete(p, vinfo->vid); >>> + if (err) >>> + break; >> >> I'm not sure a break is the right thing to do. Seems like you leave it >> in a half configured state. >> >>> if (vinfo->flags & BRIDGE_VLAN_INFO_MASTER) >>> - br_vlan_delete(p->br, vinfo->vid); >>> + err = br_vlan_delete(p->br, vinfo->vid); >>> } else { >>> - br_vlan_delete(br, vinfo->vid); >>> + err = br_vlan_delete(br, vinfo->vid); >>> } >>> break; >>> } >>> >> >> Why do you want to return the error code here? Walking the code paths >> seems like ENOENT or err from switchdev_port_obj_del are the 2 error >> possibilities. > > For example, if you attempt to delete a non-existing vlan on a port, > the current code succeeds and also sends event : > > rtnetlink_rcv_msg > rtnl_bridge_dellink > br_dellink > br_afspec > br_vlan_info > > int br_dellink(..) > { > ... > err = br_afspec() > if (err == 0) > br_ifinfo_notify(RTM_NEWLINK, p); > } > > This is misleading, so a proper errcode has to be produced. > True, but you also change the expected behaviour because now a user can clear all vlans with one request (1 - 4094), and after the change that will fail with a partial delete if some vlan was missing. This has been the behaviour forever and some script might depend on it. Also IMO, and as David also mentioned, doing a partial delete is not good.
Powered by blists - more mailing lists