| 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, 30 Aug 2012 18:58:36 +0300
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Vlad Yasevich <vyasevic@...hat.com>
Cc: netdev@...r.kernel.org
Subject: Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display
VLANS
On Thu, Aug 30, 2012 at 11:52:31AM -0400, Vlad Yasevich wrote:
> On 08/30/2012 11:47 AM, Michael S. Tsirkin wrote:
> >On Thu, Aug 30, 2012 at 11:07:24AM -0400, Vlad Yasevich wrote:
> >>On 08/30/2012 11:03 AM, Michael S. Tsirkin wrote:
> >>>On Thu, Aug 30, 2012 at 10:51:35AM -0400, Vlad Yasevich wrote:
> >>>>On 08/30/2012 10:44 AM, Michael S. Tsirkin wrote:
> >>>>>On Thu, Aug 30, 2012 at 10:36:34AM -0400, Vlad Yasevich wrote:
> >>>>>>On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
> >>>>>>>On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
> >>>>>>>>On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
> >>>>>>>>>On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
> >>>>>>>>>>Add a binary sysfs file that will dump out vlans currently configured on the
> >>>>>>>>>>port.
> >>>>>>
> >>>>>>I initially though of creating a sysfs object per vlan. That would
> >>>>>>have made it easy to see which vlans are configured without any
> >>>>>>tools.
> >>>>>>But that could result in a lot of objects being created, so I abandoned it.
> >>>>>>
> >>>>>>I did think about a text interface, but due to a page of output
> >>>>>>limitation, I didn't go that route. The reason is that if someone
> >>>>>>cats the file, they may not see all the vlans configured. So I
> >>>>>>decided on the binary interface, since a binary interface with a
> >>>>>>tool to read it could avoid the single page limitation.
> >>>>>>
> >>>>>>-vlad
> >>>>>
> >>>>>Maybe it's not needed in sysfs then - expose it to
> >>>>>brctl or whatever.
> >>>>>
> >>>>
> >>>>brctl uses sysfs for almost everything any more :)
> >>>>
> >>>>-vlad
> >>>
> >>>How about a long string of 0 and 1's?
> >>>And a separate one for untagged vlans.
> >>
> >>that would work too. You really don't like the binary interface, huh?
> >>
> >>-vlad
> >
> >Not in sysfs.
> >Another possibility: you are adding netlink command
> >to add/del a specific vid, add one to test a specific vid.
> >This means you will need to scan 4K possibilities if you
> >want to list them all, but in real life it's
> >probably just debugging tools that need to do this.
>
> Not really. A show command to see what has been configured is
> necessary not of only for debugging. It is no different then the
> show command to list fdb entries on the bridge. There needs to be
> an easy way to see which vlans are configured on which ports.
>
> -vlad
Yes. And
for (vid = 0; vid < 2096; ++vid)
ioctl(port,vid)
is easier than parsing a binary blob.
Just less efficient but that is I think OK
for the show command.
> >
> >>>
> >>>>>>>
> >>>>>>>
> >>>>>>>>>
> >>>>>>>>>>---
> >>>>>>>>>> include/linux/if_bridge.h | 1 +
> >>>>>>>>>> net/bridge/br_if.c | 34 ++++++++++++++++++++++++++++++++++
> >>>>>>>>>> net/bridge/br_private.h | 2 ++
> >>>>>>>>>> net/bridge/br_sysfs_if.c | 28 ++++++++++++++++++++++++++++
> >>>>>>>>>> 4 files changed, 65 insertions(+), 0 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> >>>>>>>>>>index ab750dd..d0f869b 100644
> >>>>>>>>>>--- a/include/linux/if_bridge.h
> >>>>>>>>>>+++ b/include/linux/if_bridge.h
> >>>>>>>>>>@@ -20,6 +20,7 @@
> >>>>>>>>>> #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
> >>>>>>>>>> #define SYSFS_BRIDGE_PORT_ATTR "brport"
> >>>>>>>>>> #define SYSFS_BRIDGE_PORT_LINK "bridge"
> >>>>>>>>>>+#define SYSFS_BRIDGE_PORT_VLANS "vlans"
> >>>>>>>>>>
> >>>>>>>>>> #define BRCTL_VERSION 1
> >>>>>>>>>>
> >>>>>>>>>>diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> >>>>>>>>>>index 90c1038..3963748 100644
> >>>>>>>>>>--- a/net/bridge/br_if.c
> >>>>>>>>>>+++ b/net/bridge/br_if.c
> >>>>>>>>>>@@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> >>>>>>>>>> return 0;
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>>+size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
> >>>>>>>>>>+ unsigned long max, unsigned long skip)
> >>>>>>>>>>+{
> >>>>>>>>>>+ unsigned long *map;
> >>>>>>>>>>+ unsigned short *vid = (unsigned short *)buf;
> >>>>>>>>>>+ unsigned short i;
> >>>>>>>>>>+ int num = 0;
> >>>>>>>>>>+
> >>>>>>>>>>+ if (skip > (VLAN_N_VID+1))
> >>>>>>>>>>+ return -EINVAL;
> >>>>>>>>>>+
> >>>>>>>>>>+ memset(buf, 0, max * sizeof(unsigned short));
> >>>>>>>>>
> >>>>>>>>>Isn't max is in bytes? why is this safe?
> >>>>>>>>>
> >>>>>>>>>>+
> >>>>>>>>>>+ rcu_read_lock();
> >>>>>>>>>>+ map = rcu_dereference(p->vlan_map);
> >>>>>>>>>>+ if (!map)
> >>>>>>>>>>+ goto out;
> >>>>>>>>>>+
> >>>>>>>>>>+ for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
> >>>>>>>>>
> >>>>>>>>>Isn't skip in bytes too? Why do you compare it to i which is
> >>>>>>>>>in dwords?
> >>>>>>>>>
> >>>>>>>>>>+ if (test_bit(i, map)) {
> >>>>>>>>>>+ if (num > max)
> >>>>>>>>>>+ goto out;
> >>>>>>>>>>+
> >>>>>>>>>>+ *vid = i-1;
> >>>>>>>>>>+ vid++;
> >>>>>>>>>>+ num++;
> >>>>>>>>>>+ }
> >>>>>>>>>>+ }
> >>>>>>>>>>+out:
> >>>>>>>>>>+ rcu_read_unlock();
> >>>>>>>>>>+
> >>>>>>>>>>+ return num*sizeof(unsigned short);
> >>>>>>>>>>+}
> >>>>>>>>>>+
> >>>>>>>>>> void __net_exit br_net_exit(struct net *net)
> >>>>>>>>>> {
> >>>>>>>>>> struct net_device *dev;
> >>>>>>>>>>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >>>>>>>>>>index 5639c1c..cf95cd7 100644
> >>>>>>>>>>--- a/net/bridge/br_private.h
> >>>>>>>>>>+++ b/net/bridge/br_private.h
> >>>>>>>>>>@@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
> >>>>>>>>>> netdev_features_t features);
> >>>>>>>>>> extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>>>>>>> extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>>>>>>>+extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
> >>>>>>>>>>+ unsigned long max, unsigned long skip);
> >>>>>>>>>>
> >>>>>>>>>> /* br_input.c */
> >>>>>>>>>> extern int br_handle_frame_finish(struct sk_buff *skb);
> >>>>>>>>>>diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> >>>>>>>>>>index 13b36bd..a81e2ef 100644
> >>>>>>>>>>--- a/net/bridge/br_sysfs_if.c
> >>>>>>>>>>+++ b/net/bridge/br_sysfs_if.c
> >>>>>>>>>>@@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
> >>>>>>>>>> };
> >>>>>>>>>>
> >>>>>>>>>> /*
> >>>>>>>>>>+ * Export the vlan table for a given port as a binary file.
> >>>>>>>>>>+ * The records are unsgined shorts.
> >>>>>>>>>>+ *
> >>>>>>>>>>+ * Returns the number of bytes read.
> >>>>>>>>>>+ */
> >>>>>>>>>>+static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
> >>>>>>>>>>+ struct bin_attribute *bin_attr,
> >>>>>>>>>>+ char *buf, loff_t off, size_t count)
> >>>>>>>>>>+{
> >>>>>>>>>>+ struct net_bridge_port *p = to_brport(kobj);
> >>>>>>>>>>+
> >>>>>>>>>>+ return br_port_fill_vlans(p, buf,
> >>>>>>>>>>+ count/sizeof(unsigned short),
> >>>>>>>>>>+ off/sizeof(unsigned short));
> >>>>>>>>>>+}
> >>>>>>>>>>+
> >>>>>>>>>>+static struct bin_attribute port_vlans = {
> >>>>>>>>>>+ .attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
> >>>>>>>>>>+ .mode = S_IRUGO, },
> >>>>>>>>>>+ .read = brport_vlans_read,
> >>>>>>>>>>+};
> >>>>>>>>>>+
> >>>>>>>>>>+/*
> >>>>>>>>>> * Add sysfs entries to ethernet device added to a bridge.
> >>>>>>>>>> * Creates a brport subdirectory with bridge attributes.
> >>>>>>>>>> * Puts symlink in bridge's brif subdirectory
> >>>>>>>>>>@@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
> >>>>>>>>>> return err;
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>>+ err = sysfs_create_bin_file(&p->kobj, &port_vlans);
> >>>>>>>>>>+ if (err) {
> >>>>>>>>>>+ return err;
> >>>>>>>>>>+ }
> >>>>>>>>>>+
> >>>>>>>>>> strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
> >>>>>>>>>> return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
> >>>>>>>>>> }
> >>>>>>>>>>--
> >>>>>>>>>>1.7.7.6
> >>>>>>>>>>
> >>>>>>>>>>--
> >>>>>>>>>>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
> >>>--
> >>>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
> >>>
--
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
Powered by blists - more mailing lists