On 06/07/2019 01:09, wenxu@xxxxxxxxx wrote:
> From: wenxu <wenxu@xxxxxxxxx>
>
> This new function allows you to fetch vlan info from packet path.
>
> Signed-off-by: wenxu <wenxu@xxxxxxxxx>
> ---
> include/linux/if_bridge.h | 7 +++++++
> net/bridge/br_vlan.c | 23 ++++++++++++++++++-----
> 2 files changed, 25 insertions(+), 5 deletions(-)
>
Hi,
This patch will need more work, comments below.
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 9e57c44..5c85608 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -92,6 +92,8 @@ static inline bool br_multicast_router(const struct net_device *dev)
> int br_vlan_get_proto(const struct net_device *dev, u16 *p_proto);
> int br_vlan_get_info(const struct net_device *dev, u16 vid,
> struct bridge_vlan_info *p_vinfo);
> +int br_vlan_get_info_rcu(const struct net_device *dev, u16 vid,
> + struct bridge_vlan_info *p_vinfo);
> #else
> static inline bool br_vlan_enabled(const struct net_device *dev)
> {
> @@ -118,6 +120,11 @@ static inline int br_vlan_get_info(const struct net_device *dev, u16 vid,
> {
> return -EINVAL;
> }
> +static inline int br_vlan_get_info_rcu(const struct net_device *dev, u16 vid,
> + struct bridge_vlan_info *p_vinfo)
> +{
> + return -EINVAL;
> +}
> #endif
>
> #if IS_ENABLED(CONFIG_BRIDGE)
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 021cc9f66..2799a88 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -1267,15 +1267,13 @@ int br_vlan_get_pvid_rcu(const struct net_device *dev, u16 *p_pvid)
> }
> EXPORT_SYMBOL_GPL(br_vlan_get_pvid_rcu);
>
> -int br_vlan_get_info(const struct net_device *dev, u16 vid,
> - struct bridge_vlan_info *p_vinfo)
> +static int __br_vlan_get_info(const struct net_device *dev, u16 vid,
> + struct net_bridge_port *p,
> + struct bridge_vlan_info *p_vinfo)
> {
> struct net_bridge_vlan_group *vg;
> struct net_bridge_vlan *v;
> - struct net_bridge_port *p;
>
> - ASSERT_RTNL();
Removing the assert here doesn't make the function proper for RCU usage.
You'll either have to split it in two and use the proper accessors to
retrieve the vlan group based on the context (rtnl or rcu) or you'll
just have to add a second version of this function which uses the proper
accessors. Also note that for the RCU version you need to check if vg
is null.
> - p = br_port_get_check_rtnl(dev);
> if (p)
> vg = nbp_vlan_group(p);
> else if (netif_is_bridge_master(dev))
> @@ -1291,8 +1289,23 @@ int br_vlan_get_info(const struct net_device *dev, u16 vid,
> p_vinfo->flags = v->flags;
> return 0;
> }
> +
> +int br_vlan_get_info(const struct net_device *dev, u16 vid,
> + struct bridge_vlan_info *p_vinfo)
> +{
> + ASSERT_RTNL();
> +
> + return __br_vlan_get_info(dev, vid, br_port_get_check_rtnl(dev), p_vinfo);
> +}
> EXPORT_SYMBOL_GPL(br_vlan_get_info);
>
> +int br_vlan_get_info_rcu(const struct net_device *dev, u16 vid,
> + struct bridge_vlan_info *p_vinfo)
> +{
> + return __br_vlan_get_info(dev, vid, br_port_get_check_rtnl(dev), p_vinfo);
> +}
> +EXPORT_SYMBOL_GPL(br_vlan_get_info_rcu);
> +
This should use br_port_get_check_rcu().
> static int br_vlan_is_bind_vlan_dev(const struct net_device *dev)
> {
> return is_vlan_dev(dev) &&
>
