* [PATCH net-next v4 1/6] net: devlink: turn devlink into a built-in
From: Jakub Kicinski @ 2019-02-26 3:34 UTC (permalink / raw)
To: davem, jiri
Cc: mkubecek, andrew, f.fainelli, netdev, oss-drivers, Jakub Kicinski
In-Reply-To: <20190226033407.32625-1-jakub.kicinski@netronome.com>
Being able to build devlink as a module causes growing pains.
First all drivers had to add a meta dependency to make sure
they are not built in when devlink is built as a module. Now
we are struggling to invoke ethtool compat code reliably.
Make devlink code built-in, users can still not build it at
all but the dynamically loadable module option is removed.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/infiniband/hw/bnxt_re/Kconfig | 1 -
drivers/infiniband/hw/mlx4/Kconfig | 1 -
drivers/net/Kconfig | 1 -
drivers/net/ethernet/broadcom/Kconfig | 1 -
drivers/net/ethernet/cavium/Kconfig | 1 -
drivers/net/ethernet/mellanox/mlx4/Kconfig | 1 -
drivers/net/ethernet/mellanox/mlx5/core/Kconfig | 1 -
drivers/net/ethernet/mellanox/mlxsw/Kconfig | 1 -
drivers/net/ethernet/netronome/Kconfig | 1 -
include/net/devlink.h | 10 ++++------
net/Kconfig | 11 +----------
net/core/devlink.c | 15 ++-------------
net/dsa/Kconfig | 2 +-
13 files changed, 8 insertions(+), 39 deletions(-)
diff --git a/drivers/infiniband/hw/bnxt_re/Kconfig b/drivers/infiniband/hw/bnxt_re/Kconfig
index 18f5ed082f41..19982a4a9bba 100644
--- a/drivers/infiniband/hw/bnxt_re/Kconfig
+++ b/drivers/infiniband/hw/bnxt_re/Kconfig
@@ -1,7 +1,6 @@
config INFINIBAND_BNXT_RE
tristate "Broadcom Netxtreme HCA support"
depends on ETHERNET && NETDEVICES && PCI && INET && DCB
- depends on MAY_USE_DEVLINK
select NET_VENDOR_BROADCOM
select BNXT
---help---
diff --git a/drivers/infiniband/hw/mlx4/Kconfig b/drivers/infiniband/hw/mlx4/Kconfig
index d1de3285fd88..4e9936731867 100644
--- a/drivers/infiniband/hw/mlx4/Kconfig
+++ b/drivers/infiniband/hw/mlx4/Kconfig
@@ -2,7 +2,6 @@ config MLX4_INFINIBAND
tristate "Mellanox ConnectX HCA support"
depends on NETDEVICES && ETHERNET && PCI && INET
depends on INFINIBAND_USER_ACCESS || !INFINIBAND_USER_ACCESS
- depends on MAY_USE_DEVLINK
select NET_VENDOR_MELLANOX
select MLX4_CORE
---help---
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 6210757772ed..5e4ca082cfcd 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -505,7 +505,6 @@ source "drivers/net/hyperv/Kconfig"
config NETDEVSIM
tristate "Simulated networking device"
depends on DEBUG_FS
- depends on MAY_USE_DEVLINK
help
This driver is a developer testing tool and software model that can
be used to test various control path networking APIs, especially
diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index c1d3ee9baf7e..716bfbba59cf 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -194,7 +194,6 @@ config SYSTEMPORT
config BNXT
tristate "Broadcom NetXtreme-C/E support"
depends on PCI
- depends on MAY_USE_DEVLINK
select FW_LOADER
select LIBCRC32C
---help---
diff --git a/drivers/net/ethernet/cavium/Kconfig b/drivers/net/ethernet/cavium/Kconfig
index 05f4a3b21e29..6650e2a5f171 100644
--- a/drivers/net/ethernet/cavium/Kconfig
+++ b/drivers/net/ethernet/cavium/Kconfig
@@ -64,7 +64,6 @@ config CAVIUM_PTP
config LIQUIDIO
tristate "Cavium LiquidIO support"
depends on 64BIT && PCI
- depends on MAY_USE_DEVLINK
depends on PCI
imply PTP_1588_CLOCK
select FW_LOADER
diff --git a/drivers/net/ethernet/mellanox/mlx4/Kconfig b/drivers/net/ethernet/mellanox/mlx4/Kconfig
index f200b8c420d5..ff8057ed97ee 100644
--- a/drivers/net/ethernet/mellanox/mlx4/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx4/Kconfig
@@ -4,7 +4,6 @@
config MLX4_EN
tristate "Mellanox Technologies 1/10/40Gbit Ethernet support"
- depends on MAY_USE_DEVLINK
depends on PCI && NETDEVICES && ETHERNET && INET
select MLX4_CORE
imply PTP_1588_CLOCK
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
index 37a551436e4a..6debffb8336b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -4,7 +4,6 @@
config MLX5_CORE
tristate "Mellanox 5th generation network adapters (ConnectX series) core driver"
- depends on MAY_USE_DEVLINK
depends on PCI
imply PTP_1588_CLOCK
imply VXLAN
diff --git a/drivers/net/ethernet/mellanox/mlxsw/Kconfig b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
index b9a25aed5d11..9c195dfed031 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
@@ -4,7 +4,6 @@
config MLXSW_CORE
tristate "Mellanox Technologies Switch ASICs support"
- depends on MAY_USE_DEVLINK
---help---
This driver supports Mellanox Technologies Switch ASICs family.
diff --git a/drivers/net/ethernet/netronome/Kconfig b/drivers/net/ethernet/netronome/Kconfig
index 66f15b05b65e..549898d5d450 100644
--- a/drivers/net/ethernet/netronome/Kconfig
+++ b/drivers/net/ethernet/netronome/Kconfig
@@ -19,7 +19,6 @@ config NFP
tristate "Netronome(R) NFP4000/NFP6000 NIC driver"
depends on PCI && PCI_MSI
depends on VXLAN || VXLAN=n
- depends on MAY_USE_DEVLINK
---help---
This driver supports the Netronome(R) NFP4000/NFP6000 based
cards working as a advanced Ethernet NIC. It works with both
diff --git a/include/net/devlink.h b/include/net/devlink.h
index a2da49dd9147..f9f7fe974652 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -707,6 +707,10 @@ devlink_health_reporter_priv(struct devlink_health_reporter *reporter);
int devlink_health_report(struct devlink_health_reporter *reporter,
const char *msg, void *priv_ctx);
+void devlink_compat_running_version(struct net_device *dev,
+ char *buf, size_t len);
+int devlink_compat_flash_update(struct net_device *dev, const char *file_name);
+
#else
static inline struct devlink *devlink_alloc(const struct devlink_ops *ops,
@@ -1190,13 +1194,7 @@ devlink_health_report(struct devlink_health_reporter *reporter,
{
return 0;
}
-#endif
-#if IS_REACHABLE(CONFIG_NET_DEVLINK)
-void devlink_compat_running_version(struct net_device *dev,
- char *buf, size_t len);
-int devlink_compat_flash_update(struct net_device *dev, const char *file_name);
-#else
static inline void
devlink_compat_running_version(struct net_device *dev, char *buf, size_t len)
{
diff --git a/net/Kconfig b/net/Kconfig
index 62da6148e9f8..1efe1f9ee492 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -429,21 +429,12 @@ config NET_SOCK_MSG
with the help of BPF programs.
config NET_DEVLINK
- tristate "Network physical/parent device Netlink interface"
+ bool "Network physical/parent device Netlink interface"
help
Network physical/parent device Netlink interface provides
infrastructure to support access to physical chip-wide config and
monitoring.
-config MAY_USE_DEVLINK
- tristate
- default m if NET_DEVLINK=m
- default y if NET_DEVLINK=y || NET_DEVLINK=n
- help
- Drivers using the devlink infrastructure should have a dependency
- on MAY_USE_DEVLINK to ensure they do not cause link errors when
- devlink is a loadable module and the driver using it is built-in.
-
config PAGE_POOL
bool
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 4f31ddc883e7..05e04ea0a5c7 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6482,20 +6482,9 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
return -EOPNOTSUPP;
}
-static int __init devlink_module_init(void)
+static int __init devlink_init(void)
{
return genl_register_family(&devlink_nl_family);
}
-static void __exit devlink_module_exit(void)
-{
- genl_unregister_family(&devlink_nl_family);
-}
-
-module_init(devlink_module_init);
-module_exit(devlink_module_exit);
-
-MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR("Jiri Pirko <jiri@mellanox.com>");
-MODULE_DESCRIPTION("Network physical device Netlink interface");
-MODULE_ALIAS_GENL_FAMILY(DEVLINK_GENL_NAME);
+subsys_initcall(devlink_init);
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 91e52973ee13..fab49132345f 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -6,7 +6,7 @@ config HAVE_NET_DSA
config NET_DSA
tristate "Distributed Switch Architecture"
- depends on HAVE_NET_DSA && MAY_USE_DEVLINK
+ depends on HAVE_NET_DSA
depends on BRIDGE || BRIDGE=n
select NET_SWITCHDEV
select PHYLINK
--
2.19.2
^ permalink raw reply related
* [PATCH net-next v4 2/6] devlink: create a special NDO for getting the devlink instance
From: Jakub Kicinski @ 2019-02-26 3:34 UTC (permalink / raw)
To: davem, jiri
Cc: mkubecek, andrew, f.fainelli, netdev, oss-drivers, Jakub Kicinski
In-Reply-To: <20190226033407.32625-1-jakub.kicinski@netronome.com>
Instead of iterating over all devlink ports add a NDO which
will return the devlink instance from the driver.
v2: add the netdev_to_devlink() helper (Michal)
v3: check that devlink has ops (Florian)
v4: hold devlink_mutex (Jiri)
Suggested-by: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
include/linux/netdevice.h | 7 +++++
include/net/devlink.h | 9 +++++++
net/core/devlink.c | 56 ++++++++++++---------------------------
3 files changed, 33 insertions(+), 39 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ffbddd03242b..58e83bd7a861 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -941,6 +941,8 @@ struct dev_ifalias {
char ifalias[];
};
+struct devlink;
+
/*
* This structure defines the management hooks for network devices.
* The following hooks can be defined; unless noted otherwise, they are
@@ -1249,6 +1251,10 @@ struct dev_ifalias {
* that got dropped are freed/returned via xdp_return_frame().
* Returns negative number, means general error invoking ndo, meaning
* no frames were xmit'ed and core-caller will free all frames.
+ * struct devlink *(*ndo_get_devlink)(struct net_device *dev);
+ * Get devlink instance associated with a given netdev.
+ * Called with a reference on the netdevice and devlink locks only,
+ * rtnl_lock is not held.
*/
struct net_device_ops {
int (*ndo_init)(struct net_device *dev);
@@ -1447,6 +1453,7 @@ struct net_device_ops {
u32 flags);
int (*ndo_xsk_async_xmit)(struct net_device *dev,
u32 queue_id);
+ struct devlink * (*ndo_get_devlink)(struct net_device *dev);
};
/**
diff --git a/include/net/devlink.h b/include/net/devlink.h
index f9f7fe974652..7f5a0bdca228 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -538,6 +538,15 @@ static inline struct devlink *priv_to_devlink(void *priv)
return container_of(priv, struct devlink, priv);
}
+static inline struct devlink *netdev_to_devlink(struct net_device *dev)
+{
+#if IS_ENABLED(CONFIG_NET_DEVLINK)
+ if (dev->netdev_ops->ndo_get_devlink)
+ return dev->netdev_ops->ndo_get_devlink(dev);
+#endif
+ return NULL;
+}
+
struct ib_device;
#if IS_ENABLED(CONFIG_NET_DEVLINK)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 05e04ea0a5c7..24bfbd2d71e7 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6397,9 +6397,6 @@ static void __devlink_compat_running_version(struct devlink *devlink,
struct sk_buff *msg;
int rem, err;
- if (!devlink->ops->info_get)
- return;
-
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!msg)
return;
@@ -6431,55 +6428,36 @@ static void __devlink_compat_running_version(struct devlink *devlink,
void devlink_compat_running_version(struct net_device *dev,
char *buf, size_t len)
{
- struct devlink_port *devlink_port;
struct devlink *devlink;
mutex_lock(&devlink_mutex);
- list_for_each_entry(devlink, &devlink_list, list) {
- mutex_lock(&devlink->lock);
- list_for_each_entry(devlink_port, &devlink->port_list, list) {
- if (devlink_port->type == DEVLINK_PORT_TYPE_ETH &&
- devlink_port->type_dev == dev) {
- __devlink_compat_running_version(devlink,
- buf, len);
- mutex_unlock(&devlink->lock);
- goto out;
- }
- }
- mutex_unlock(&devlink->lock);
- }
-out:
+ devlink = netdev_to_devlink(dev);
+ if (!devlink || !devlink->ops || !devlink->ops->info_get)
+ goto unlock_list;
+
+ mutex_lock(&devlink->lock);
+ __devlink_compat_running_version(devlink, buf, len);
+ mutex_unlock(&devlink->lock);
+unlock_list:
mutex_unlock(&devlink_mutex);
}
int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
{
- struct devlink_port *devlink_port;
struct devlink *devlink;
+ int ret = -EOPNOTSUPP;
mutex_lock(&devlink_mutex);
- list_for_each_entry(devlink, &devlink_list, list) {
- mutex_lock(&devlink->lock);
- list_for_each_entry(devlink_port, &devlink->port_list, list) {
- int ret = -EOPNOTSUPP;
-
- if (devlink_port->type != DEVLINK_PORT_TYPE_ETH ||
- devlink_port->type_dev != dev)
- continue;
+ devlink = netdev_to_devlink(dev);
+ if (!devlink || !devlink->ops || !devlink->ops->flash_update)
+ goto unlock_list;
- mutex_unlock(&devlink_mutex);
- if (devlink->ops->flash_update)
- ret = devlink->ops->flash_update(devlink,
- file_name,
- NULL, NULL);
- mutex_unlock(&devlink->lock);
- return ret;
- }
- mutex_unlock(&devlink->lock);
- }
+ mutex_lock(&devlink->lock);
+ ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL);
+ mutex_unlock(&devlink->lock);
+unlock_list:
mutex_unlock(&devlink_mutex);
-
- return -EOPNOTSUPP;
+ return ret;
}
static int __init devlink_init(void)
--
2.19.2
^ permalink raw reply related
* [PATCH net-next v4 3/6] nfp: add .ndo_get_devlink
From: Jakub Kicinski @ 2019-02-26 3:34 UTC (permalink / raw)
To: davem, jiri
Cc: mkubecek, andrew, f.fainelli, netdev, oss-drivers, Jakub Kicinski
In-Reply-To: <20190226033407.32625-1-jakub.kicinski@netronome.com>
Support getting devlink instance from a new NDO.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/netronome/nfp/nfp_app.h | 2 ++
drivers/net/ethernet/netronome/nfp/nfp_devlink.c | 11 +++++++++++
drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 1 +
drivers/net/ethernet/netronome/nfp/nfp_net_repr.c | 1 +
4 files changed, 15 insertions(+)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_app.h b/drivers/net/ethernet/netronome/nfp/nfp_app.h
index d578d856a009..f8d422713705 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_app.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_app.h
@@ -433,4 +433,6 @@ int nfp_app_nic_vnic_alloc(struct nfp_app *app, struct nfp_net *nn,
int nfp_app_nic_vnic_init_phy_port(struct nfp_pf *pf, struct nfp_app *app,
struct nfp_net *nn, unsigned int id);
+struct devlink *nfp_devlink_get_devlink(struct net_device *netdev);
+
#endif
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index db2da99f6aa7..e9eca99cf493 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -376,3 +376,14 @@ void nfp_devlink_port_unregister(struct nfp_port *port)
{
devlink_port_unregister(&port->dl_port);
}
+
+struct devlink *nfp_devlink_get_devlink(struct net_device *netdev)
+{
+ struct nfp_app *app;
+
+ app = nfp_app_from_netdev(netdev);
+ if (!app)
+ return NULL;
+
+ return priv_to_devlink(app->pf);
+}
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 776f6c07701b..6d1b8816552e 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -3531,6 +3531,7 @@ const struct net_device_ops nfp_net_netdev_ops = {
.ndo_udp_tunnel_del = nfp_net_del_vxlan_port,
.ndo_bpf = nfp_net_xdp,
.ndo_get_port_parent_id = nfp_port_get_port_parent_id,
+ .ndo_get_devlink = nfp_devlink_get_devlink,
};
/**
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
index 62839807e21e..d2c803bb4e56 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
@@ -273,6 +273,7 @@ const struct net_device_ops nfp_repr_netdev_ops = {
.ndo_set_features = nfp_port_set_features,
.ndo_set_mac_address = eth_mac_addr,
.ndo_get_port_parent_id = nfp_port_get_port_parent_id,
+ .ndo_get_devlink = nfp_devlink_get_devlink,
};
void
--
2.19.2
^ permalink raw reply related
* [PATCH net-next v4 4/6] nfp: remove ethtool flashing fallback
From: Jakub Kicinski @ 2019-02-26 3:34 UTC (permalink / raw)
To: davem, jiri
Cc: mkubecek, andrew, f.fainelli, netdev, oss-drivers, Jakub Kicinski
In-Reply-To: <20190226033407.32625-1-jakub.kicinski@netronome.com>
Now that devlink fallback will be called reliably, we can remove
the ethtool flashing code.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
.../ethernet/netronome/nfp/nfp_net_ethtool.c | 24 -------------------
1 file changed, 24 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index 8f189149efc5..690b62718dbb 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -1234,28 +1234,6 @@ static int nfp_net_set_channels(struct net_device *netdev,
return nfp_net_set_num_rings(nn, total_rx, total_tx);
}
-static int
-nfp_net_flash_device(struct net_device *netdev, struct ethtool_flash *flash)
-{
- struct nfp_app *app;
- int ret;
-
- if (flash->region != ETHTOOL_FLASH_ALL_REGIONS)
- return -EOPNOTSUPP;
-
- app = nfp_app_from_netdev(netdev);
- if (!app)
- return -EOPNOTSUPP;
-
- dev_hold(netdev);
- rtnl_unlock();
- ret = nfp_flash_update_common(app->pf, flash->data, NULL);
- rtnl_lock();
- dev_put(netdev);
-
- return ret;
-}
-
static const struct ethtool_ops nfp_net_ethtool_ops = {
.get_drvinfo = nfp_net_get_drvinfo,
.get_link = ethtool_op_get_link,
@@ -1266,7 +1244,6 @@ static const struct ethtool_ops nfp_net_ethtool_ops = {
.get_sset_count = nfp_net_get_sset_count,
.get_rxnfc = nfp_net_get_rxnfc,
.set_rxnfc = nfp_net_set_rxnfc,
- .flash_device = nfp_net_flash_device,
.get_rxfh_indir_size = nfp_net_get_rxfh_indir_size,
.get_rxfh_key_size = nfp_net_get_rxfh_key_size,
.get_rxfh = nfp_net_get_rxfh,
@@ -1292,7 +1269,6 @@ const struct ethtool_ops nfp_port_ethtool_ops = {
.get_strings = nfp_port_get_strings,
.get_ethtool_stats = nfp_port_get_stats,
.get_sset_count = nfp_port_get_sset_count,
- .flash_device = nfp_net_flash_device,
.set_dump = nfp_app_set_dump,
.get_dump_flag = nfp_app_get_dump_flag,
.get_dump_data = nfp_app_get_dump_data,
--
2.19.2
^ permalink raw reply related
* [PATCH net-next v4 5/6] devlink: hold a reference to the netdevice around ethtool compat
From: Jakub Kicinski @ 2019-02-26 3:34 UTC (permalink / raw)
To: davem, jiri
Cc: mkubecek, andrew, f.fainelli, netdev, oss-drivers, Jakub Kicinski
In-Reply-To: <20190226033407.32625-1-jakub.kicinski@netronome.com>
When ethtool is calling into devlink compat code make sure we have
a reference on the netdevice on which the operation was invoked.
v3: move the hold/lock logic into devlink_compat_* functions (Florian)
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
net/core/devlink.c | 13 +++++++++++++
net/core/ethtool.c | 13 ++-----------
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 24bfbd2d71e7..ecff6e63bc4d 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6430,6 +6430,9 @@ void devlink_compat_running_version(struct net_device *dev,
{
struct devlink *devlink;
+ dev_hold(dev);
+ rtnl_unlock();
+
mutex_lock(&devlink_mutex);
devlink = netdev_to_devlink(dev);
if (!devlink || !devlink->ops || !devlink->ops->info_get)
@@ -6440,6 +6443,9 @@ void devlink_compat_running_version(struct net_device *dev,
mutex_unlock(&devlink->lock);
unlock_list:
mutex_unlock(&devlink_mutex);
+
+ rtnl_lock();
+ dev_put(dev);
}
int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
@@ -6447,6 +6453,9 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
struct devlink *devlink;
int ret = -EOPNOTSUPP;
+ dev_hold(dev);
+ rtnl_unlock();
+
mutex_lock(&devlink_mutex);
devlink = netdev_to_devlink(dev);
if (!devlink || !devlink->ops || !devlink->ops->flash_update)
@@ -6457,6 +6466,10 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
mutex_unlock(&devlink->lock);
unlock_list:
mutex_unlock(&devlink_mutex);
+
+ rtnl_lock();
+ dev_put(dev);
+
return ret;
}
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1320e8dce559..47558ffae423 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -805,11 +805,9 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
if (ops->get_eeprom_len)
info.eedump_len = ops->get_eeprom_len(dev);
- rtnl_unlock();
if (!info.fw_version[0])
devlink_compat_running_version(dev, info.fw_version,
sizeof(info.fw_version));
- rtnl_lock();
if (copy_to_user(useraddr, &info, sizeof(info)))
return -EFAULT;
@@ -2040,15 +2038,8 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
return -EFAULT;
efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
- if (!dev->ethtool_ops->flash_device) {
- int ret;
-
- rtnl_unlock();
- ret = devlink_compat_flash_update(dev, efl.data);
- rtnl_lock();
-
- return ret;
- }
+ if (!dev->ethtool_ops->flash_device)
+ return devlink_compat_flash_update(dev, efl.data);
return dev->ethtool_ops->flash_device(dev, &efl);
}
--
2.19.2
^ permalink raw reply related
* [PATCH net-next v4 6/6] devlink: require non-NULL ops for devlink instances
From: Jakub Kicinski @ 2019-02-26 3:34 UTC (permalink / raw)
To: davem, jiri
Cc: mkubecek, andrew, f.fainelli, netdev, oss-drivers, Jakub Kicinski
In-Reply-To: <20190226033407.32625-1-jakub.kicinski@netronome.com>
Commit 76726ccb7f46 ("devlink: add flash update command") and
commit 2d8dc5bbf4e7 ("devlink: Add support for reload")
access devlink ops without NULL-checking. There is, however, no
driver which would pass in NULL ops, so let's just make that
a requirement. Remove the now unnecessary NULL-checking.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
net/core/devlink.c | 48 +++++++++++++++++++++-------------------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index ecff6e63bc4d..a49dee67e66f 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -723,7 +723,7 @@ static int devlink_port_type_set(struct devlink *devlink,
{
int err;
- if (devlink->ops && devlink->ops->port_type_set) {
+ if (devlink->ops->port_type_set) {
if (port_type == DEVLINK_PORT_TYPE_NOTSET)
return -EINVAL;
if (port_type == devlink_port->type)
@@ -760,7 +760,7 @@ static int devlink_port_split(struct devlink *devlink, u32 port_index,
u32 count, struct netlink_ext_ack *extack)
{
- if (devlink->ops && devlink->ops->port_split)
+ if (devlink->ops->port_split)
return devlink->ops->port_split(devlink, port_index, count,
extack);
return -EOPNOTSUPP;
@@ -786,7 +786,7 @@ static int devlink_port_unsplit(struct devlink *devlink, u32 port_index,
struct netlink_ext_ack *extack)
{
- if (devlink->ops && devlink->ops->port_unsplit)
+ if (devlink->ops->port_unsplit)
return devlink->ops->port_unsplit(devlink, port_index, extack);
return -EOPNOTSUPP;
}
@@ -961,7 +961,7 @@ static int devlink_nl_cmd_sb_pool_get_doit(struct sk_buff *skb,
if (err)
return err;
- if (!devlink->ops || !devlink->ops->sb_pool_get)
+ if (!devlink->ops->sb_pool_get)
return -EOPNOTSUPP;
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
@@ -1017,7 +1017,7 @@ static int devlink_nl_cmd_sb_pool_get_dumpit(struct sk_buff *msg,
mutex_lock(&devlink_mutex);
list_for_each_entry(devlink, &devlink_list, list) {
if (!net_eq(devlink_net(devlink), sock_net(msg->sk)) ||
- !devlink->ops || !devlink->ops->sb_pool_get)
+ !devlink->ops->sb_pool_get)
continue;
mutex_lock(&devlink->lock);
list_for_each_entry(devlink_sb, &devlink->sb_list, list) {
@@ -1046,7 +1046,7 @@ static int devlink_sb_pool_set(struct devlink *devlink, unsigned int sb_index,
{
const struct devlink_ops *ops = devlink->ops;
- if (ops && ops->sb_pool_set)
+ if (ops->sb_pool_set)
return ops->sb_pool_set(devlink, sb_index, pool_index,
size, threshold_type);
return -EOPNOTSUPP;
@@ -1151,7 +1151,7 @@ static int devlink_nl_cmd_sb_port_pool_get_doit(struct sk_buff *skb,
if (err)
return err;
- if (!devlink->ops || !devlink->ops->sb_port_pool_get)
+ if (!devlink->ops->sb_port_pool_get)
return -EOPNOTSUPP;
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
@@ -1213,7 +1213,7 @@ static int devlink_nl_cmd_sb_port_pool_get_dumpit(struct sk_buff *msg,
mutex_lock(&devlink_mutex);
list_for_each_entry(devlink, &devlink_list, list) {
if (!net_eq(devlink_net(devlink), sock_net(msg->sk)) ||
- !devlink->ops || !devlink->ops->sb_port_pool_get)
+ !devlink->ops->sb_port_pool_get)
continue;
mutex_lock(&devlink->lock);
list_for_each_entry(devlink_sb, &devlink->sb_list, list) {
@@ -1242,7 +1242,7 @@ static int devlink_sb_port_pool_set(struct devlink_port *devlink_port,
{
const struct devlink_ops *ops = devlink_port->devlink->ops;
- if (ops && ops->sb_port_pool_set)
+ if (ops->sb_port_pool_set)
return ops->sb_port_pool_set(devlink_port, sb_index,
pool_index, threshold);
return -EOPNOTSUPP;
@@ -1355,7 +1355,7 @@ static int devlink_nl_cmd_sb_tc_pool_bind_get_doit(struct sk_buff *skb,
if (err)
return err;
- if (!devlink->ops || !devlink->ops->sb_tc_pool_bind_get)
+ if (!devlink->ops->sb_tc_pool_bind_get)
return -EOPNOTSUPP;
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
@@ -1439,7 +1439,7 @@ devlink_nl_cmd_sb_tc_pool_bind_get_dumpit(struct sk_buff *msg,
mutex_lock(&devlink_mutex);
list_for_each_entry(devlink, &devlink_list, list) {
if (!net_eq(devlink_net(devlink), sock_net(msg->sk)) ||
- !devlink->ops || !devlink->ops->sb_tc_pool_bind_get)
+ !devlink->ops->sb_tc_pool_bind_get)
continue;
mutex_lock(&devlink->lock);
@@ -1471,7 +1471,7 @@ static int devlink_sb_tc_pool_bind_set(struct devlink_port *devlink_port,
{
const struct devlink_ops *ops = devlink_port->devlink->ops;
- if (ops && ops->sb_tc_pool_bind_set)
+ if (ops->sb_tc_pool_bind_set)
return ops->sb_tc_pool_bind_set(devlink_port, sb_index,
tc_index, pool_type,
pool_index, threshold);
@@ -1519,7 +1519,7 @@ static int devlink_nl_cmd_sb_occ_snapshot_doit(struct sk_buff *skb,
struct devlink_sb *devlink_sb = info->user_ptr[1];
const struct devlink_ops *ops = devlink->ops;
- if (ops && ops->sb_occ_snapshot)
+ if (ops->sb_occ_snapshot)
return ops->sb_occ_snapshot(devlink, devlink_sb->index);
return -EOPNOTSUPP;
}
@@ -1531,7 +1531,7 @@ static int devlink_nl_cmd_sb_occ_max_clear_doit(struct sk_buff *skb,
struct devlink_sb *devlink_sb = info->user_ptr[1];
const struct devlink_ops *ops = devlink->ops;
- if (ops && ops->sb_occ_max_clear)
+ if (ops->sb_occ_max_clear)
return ops->sb_occ_max_clear(devlink, devlink_sb->index);
return -EOPNOTSUPP;
}
@@ -1594,13 +1594,9 @@ static int devlink_nl_cmd_eswitch_get_doit(struct sk_buff *skb,
struct genl_info *info)
{
struct devlink *devlink = info->user_ptr[0];
- const struct devlink_ops *ops = devlink->ops;
struct sk_buff *msg;
int err;
- if (!ops)
- return -EOPNOTSUPP;
-
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!msg)
return -ENOMEM;
@@ -1625,9 +1621,6 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
int err = 0;
u16 mode;
- if (!ops)
- return -EOPNOTSUPP;
-
if (info->attrs[DEVLINK_ATTR_ESWITCH_MODE]) {
if (!ops->eswitch_mode_set)
return -EOPNOTSUPP;
@@ -3869,7 +3862,7 @@ static int devlink_nl_cmd_info_get_doit(struct sk_buff *skb,
struct sk_buff *msg;
int err;
- if (!devlink->ops || !devlink->ops->info_get)
+ if (!devlink->ops->info_get)
return -EOPNOTSUPP;
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
@@ -5232,6 +5225,9 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size)
{
struct devlink *devlink;
+ if (WARN_ON(!ops))
+ return NULL;
+
devlink = kzalloc(sizeof(*devlink) + priv_size, GFP_KERNEL);
if (!devlink)
return NULL;
@@ -6091,7 +6087,7 @@ __devlink_param_driverinit_value_set(struct devlink *devlink,
int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
union devlink_param_value *init_val)
{
- if (!devlink->ops || !devlink->ops->reload)
+ if (!devlink->ops->reload)
return -EOPNOTSUPP;
return __devlink_param_driverinit_value_get(&devlink->param_list,
@@ -6138,7 +6134,7 @@ int devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
{
struct devlink *devlink = devlink_port->devlink;
- if (!devlink->ops || !devlink->ops->reload)
+ if (!devlink->ops->reload)
return -EOPNOTSUPP;
return __devlink_param_driverinit_value_get(&devlink_port->param_list,
@@ -6435,7 +6431,7 @@ void devlink_compat_running_version(struct net_device *dev,
mutex_lock(&devlink_mutex);
devlink = netdev_to_devlink(dev);
- if (!devlink || !devlink->ops || !devlink->ops->info_get)
+ if (!devlink || !devlink->ops->info_get)
goto unlock_list;
mutex_lock(&devlink->lock);
@@ -6458,7 +6454,7 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
mutex_lock(&devlink_mutex);
devlink = netdev_to_devlink(dev);
- if (!devlink || !devlink->ops || !devlink->ops->flash_update)
+ if (!devlink || !devlink->ops->flash_update)
goto unlock_list;
mutex_lock(&devlink->lock);
--
2.19.2
^ permalink raw reply related
* Re: [PATCH v3 bpf-next 1/4] bpf: enable program stats
From: Alexei Starovoitov @ 2019-02-26 3:42 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Alexei Starovoitov, davem@davemloft.net, daniel@iogearbox.net,
edumazet@google.com, netdev@vger.kernel.org, bpf@vger.kernel.org,
Kernel Team
In-Reply-To: <20190226031049.GD32115@mini-arch>
On 2/25/19 7:10 PM, Stanislav Fomichev wrote:
> On 02/25, Alexei Starovoitov wrote:
>> On 2/25/19 3:07 PM, Stanislav Fomichev wrote:
>>>> +#define BPF_PROG_RUN(prog, ctx) ({ \
>>>> + u32 ret; \
>>>> + cant_sleep(); \
>>>> + if (static_branch_unlikely(&bpf_stats_enabled_key)) { \
>>>> + struct bpf_prog_stats *stats; \
>>>> + u64 start = sched_clock(); \
>>> QQ: why sched_clock() and not, for example, ktime_get_ns() which we do
>>> in the bpf_test_run()? Or even why not local_clock?
>>> I'm just wondering what king of trade off we are doing here
>>> regarding precision vs run time cost.
>>
>>
>> I'm making this decision based on documentation:
>> Documentation/timers/timekeeping.txt
>> "Compared to clock sources, sched_clock() has to be very fast: it is
>> called much more often, especially by the scheduler. If you have to do
>> trade-offs between accuracy compared to the clock source, you may
>> sacrifice accuracy for speed in sched_clock()."
> So sched_clock is fast, but imprecise; and ktime_get_ns (and
> lock_clock?) are slow(er), but more precise?
>
> If that's the case, would it make sense to use a more precise
> measurement? I suppose the BPF program execution time is on the order of
> nanoseconds and if sched_close has msec or usec resolution, all we get is
> essentially noise?
>
> I understand that you want this feature to have almost no overhead, but
> since it's gated by the static key, should we aim for a higher precision?
>
Considering everything I believe sched_clock() strikes the best trade off.
^ permalink raw reply
* Re: [PATCH net] ipv4: Add ICMPv6 support when parse route ipproto
From: Hangbin Liu @ 2019-02-26 3:48 UTC (permalink / raw)
To: David Ahern; +Cc: Sabrina Dubroca, netdev, Roopa Prabhu, David S . Miller
In-Reply-To: <20f3652d-479b-7be1-cf13-e7d8a61dca9e@gmail.com>
Hi David,
On Mon, Feb 25, 2019 at 07:23:33PM -0700, David Ahern wrote:
> On 2/25/19 7:17 PM, Hangbin Liu wrote:
> > I also thought about this issue. Currently we didn't check the ipproto in both
> > IPv4 and IPv6. You can set icmp in ip6 rules or icmpv6 in ipv4 rules.
> > This looks don't make any serious problem. It's just a user mis-configuration,
> > the kernel check the proto number and won't match normal IP/IPv6 headers.
> >
> > But yes, we should make it more strict, do you think if I should add a new
> > rtm_getroute_parse_ip6_proto() function, or just add a family parameter
> > in previous function?
>
> I see now. rtm_getroute_parse_ip_proto is used for ipv4 and ipv6. For v4
> IPPROTO_ICMPV6 should not be allowed and for v6 IPPROTO_ICMP should
> fail. You could a version argument to rtm_getroute_parse_ip_proto and
> fail as needed.
Sorry I didn't get here. Do you mean add an IPv6 version of
rtm_getroute_parse_ip_proto?
Thanks
Hangbin
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Christoph Hellwig @ 2019-02-26 3:57 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Miller, Alexei Starovoitov, Masami Hiramatsu,
Steven Rostedt, Andy Lutomirski, Linux List Kernel Mailing,
Ingo Molnar, Andrew Morton, stable, Changbin Du, Jann Horn,
Kees Cook, Andrew Lutomirski, Daniel Borkmann, Netdev, bpf
In-Reply-To: <CAHk-=wh9uWJaih5i6qBPer5rzcs=A+yb_ejU093b1esAhoFveQ@mail.gmail.com>
On Fri, Feb 22, 2019 at 01:59:10PM -0800, Linus Torvalds wrote:
> On Fri, Feb 22, 2019 at 1:38 PM David Miller <davem@davemloft.net> wrote:
> >
> > Don't be surprised if we see more separation like this in the future too.
>
> Yes, with the whole meltdown fiasco, there's actually more pressure to
> add more support for separation of kernel/user address spaces. As Andy
> pointed out, it's been discussed as a future wish-list for x86-64 too.
S/390 is another example.
I've also proposed a RISC-V extension for it, including prototypes
for Rocketchip and Qemu, and a Linux kernel support, but it didn't go
any way.
^ permalink raw reply
* Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled
From: Hangbin Liu @ 2019-02-26 4:08 UTC (permalink / raw)
To: David Ahern; +Cc: Eric Dumazet, netdev, Stefano Brivio, David S . Miller
In-Reply-To: <da79cacd-c95d-6f52-109e-10273f7452b3@cumulusnetworks.com>
Hi David,
On Mon, Feb 25, 2019 at 07:15:26PM -0700, David Ahern wrote:
> On 2/25/19 6:55 PM, Hangbin Liu wrote:
> > Just as I said, this issue only occurs when IPv6 is disabled at boot time
> > as there is no IPv6 route entry. Disable ipv6 on specific interface should
> > not be affected(IPv6 route/fib has inited). So I think use ipv6_mod_enabled()
> > would be more suitable in this scenario. Did I miss something?
>
> From a readability perspective the code path depends on whether ipv6 is
> enabled on the device. I think it is better to leave that as it is.
When I posted 173656accaf5 ("sit: check if IPv6 enabled before calling
ip6_err_gen_icmpv6_unreach()"), my purpose is to check if IPv6 disabled
at boot time. I didn't know we have ipv6_mod_enabled() at that time, so I
just used __in6_dev_get() as a trick way/work around.
A few days later I saw your commit e434863718d4 ("net: vrf: Fix crash when
IPv6 is disabled at boot time") and I thought this would be a more clear way
to tell people that we are checking if IPv6 disabled at boot time. So I posted
these two follow up fixes.
I don't know why you thought check IPv6 is enbled on the device is better.
Because it's more strict? Maybe I missed something here. But if you feel it
is better to leave as it it, then let's keep it.
Thanks
Hangbin
^ permalink raw reply
* Re: [PATCH net-next v4 5/6] devlink: hold a reference to the netdevice around ethtool compat
From: Florian Fainelli @ 2019-02-26 4:26 UTC (permalink / raw)
To: Jakub Kicinski, davem, jiri; +Cc: mkubecek, andrew, netdev, oss-drivers
In-Reply-To: <20190226033407.32625-6-jakub.kicinski@netronome.com>
On 2/25/2019 7:34 PM, Jakub Kicinski wrote:
> When ethtool is calling into devlink compat code make sure we have
> a reference on the netdevice on which the operation was invoked.
>
> v3: move the hold/lock logic into devlink_compat_* functions (Florian)
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH bpf-next 1/4] bpf: enable program stats
From: Alexei Starovoitov @ 2019-02-26 4:27 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov
Cc: Roman Gushchin, Alexei Starovoitov, davem@davemloft.net,
netdev@vger.kernel.org, bpf@vger.kernel.org, Kernel Team
In-Reply-To: <439aa0d4-3f31-1385-076a-bb8dab515bef@iogearbox.net>
On 2/25/19 2:36 AM, Daniel Borkmann wrote:
>
> Not through the stack, but was more thinking something like low-overhead
> kprobes-style extension for a BPF prog where such sequence would be added
> 'inline' at beginning / exit of BPF prog invocation with normal ctx access
> and helpers as the native program (e.g. time-stamping plus read/write into
> mark as one example which kprobes couldn't do). But might go much beyond
> context of this stats collection.
see below.
> That's actually an interesting thought, given the original prog->bpf_func
> is a known address at that time, this could be templated where an inner
> dummy bpf_func call to the normal BPF prog gets later replaced with the
> actual prog->bpf_func address to have no indirect call. This would also
> allow to keep the actual prog->bpf_func entry call-sites small and simple.
My first thought was that we indeed can have a template of stats
collecting wrapper in .text,
then at 'switch stats on' event vmalloc exec region, copy wrapper code
into it and manually patch 'call foo' x86 insn with direct jump.
That is still quite involved from coding side.
It's similar to what kprobe/ftrace is doing, but probably
an overkill for stats due to potential security
concerns with new executable code.
But it won't work due to tail_calls.
I've looked into single wrapper approach with indirect call
(because it's much easer than patching x86) and realized it
won't work for the same reason.
Inline kprobe+kretprobe insertion won't work either.
All because we have tail_calls.
we cannot have executable epilogue in progs.
tail_call will jmp into next prog and second part of stats collection
won't run. Essentially no epilogue allowed unless we disable tail_call.
Or we somehow reimplement tail_calls so that prog returns,
an epilogue is executed and then jumps into the next prog.
tail_call turned out be the worst corner case feature to deal with.
With static_key approach the main prog accounts the time
of all progs called via tail_call.
We can argue whether it's ideal semantics, but looks like
it's the only thing we can do.
I don't see a way how tail_called progs can count their own time.
Stats would somehow need to be computed right before jumping
into next prog. Which means heavy changes in all JITs
plus extra performance cost at runtime, since such if (stats_on)
check in tail_call handling would have to be done at runtime,
since JITed code is read-only and regenerating progs due to stats
is non-starter.
Or tail-called next prog would need to update stats for previous
prog, but there is no pointer to 'aux->stats' there.
So imo that is dead end.
static_key approach is the simplest by far.
^ permalink raw reply
* Re: [PATCH net-next v4 6/6] devlink: require non-NULL ops for devlink instances
From: Florian Fainelli @ 2019-02-26 4:29 UTC (permalink / raw)
To: Jakub Kicinski, davem, jiri; +Cc: mkubecek, andrew, netdev, oss-drivers
In-Reply-To: <20190226033407.32625-7-jakub.kicinski@netronome.com>
On 2/25/2019 7:34 PM, Jakub Kicinski wrote:
> Commit 76726ccb7f46 ("devlink: add flash update command") and
> commit 2d8dc5bbf4e7 ("devlink: Add support for reload")
> access devlink ops without NULL-checking. There is, however, no
> driver which would pass in NULL ops, so let's just make that
> a requirement. Remove the now unnecessary NULL-checking.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled
From: Eric Dumazet @ 2019-02-26 4:36 UTC (permalink / raw)
To: Hangbin Liu, David Ahern; +Cc: netdev, Stefano Brivio, David S . Miller
In-Reply-To: <20190226040759.GU10051@dhcp-12-139.nay.redhat.com>
On 02/25/2019 08:08 PM, Hangbin Liu wrote:
> Hi David,
> On Mon, Feb 25, 2019 at 07:15:26PM -0700, David Ahern wrote:
>> On 2/25/19 6:55 PM, Hangbin Liu wrote:
>>> Just as I said, this issue only occurs when IPv6 is disabled at boot time
>>> as there is no IPv6 route entry. Disable ipv6 on specific interface should
>>> not be affected(IPv6 route/fib has inited). So I think use ipv6_mod_enabled()
>>> would be more suitable in this scenario. Did I miss something?
>>
>> From a readability perspective the code path depends on whether ipv6 is
>> enabled on the device. I think it is better to leave that as it is.
>
> When I posted 173656accaf5 ("sit: check if IPv6 enabled before calling
> ip6_err_gen_icmpv6_unreach()"), my purpose is to check if IPv6 disabled
> at boot time. I didn't know we have ipv6_mod_enabled() at that time, so I
> just used __in6_dev_get() as a trick way/work around.
>
> A few days later I saw your commit e434863718d4 ("net: vrf: Fix crash when
> IPv6 is disabled at boot time") and I thought this would be a more clear way
> to tell people that we are checking if IPv6 disabled at boot time. So I posted
> these two follow up fixes.
>
> I don't know why you thought check IPv6 is enbled on the device is better.
> Because it's more strict? Maybe I missed something here. But if you feel it
> is better to leave as it it, then let's keep it.
>
I do not get it.
We really do not care if IPv6 is enabled on some device on the host.
Here the correct test is checking if IPv6 is enabled for _this_ device.
It is not about fixing a crash (it is already fixed), but not having to call
ip6_err_gen_icmpv6_unreach() knowing it will fail anyway.
So the current code is better.
Thank you.
^ permalink raw reply
* Re: [PATCH] tools: testing: selftests: Remove duplicate headers
From: Souptick Joarder @ 2019-02-26 5:29 UTC (permalink / raw)
To: Michael Ellerman
Cc: Bamvor Zhang, shuah, David S. Miller, Benjamin Herrenschmidt,
Paul Mackerras, Alexey Dobriyan, Mathieu Desnoyers,
Peter Zijlstra, Paul McKenney, boqun.feng, John Stultz,
Thomas Gleixner, sboyd, Andrew Morton, linux-gpio,
linux-kselftest, linux-kernel, netdev, linuxppc-dev,
linux-fsdevel, Sabyasachi Gupta
In-Reply-To: <87tvgr1fr4.fsf@concordia.ellerman.id.au>
On Tue, Feb 26, 2019 at 7:18 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Souptick Joarder <jrdr.linux@gmail.com> writes:
> > Remove duplicate headers which are included twice.
> >
> > Signed-off-by: Sabyasachi Gupta <sabyasachi.linux@gmail.com>
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > ---
> ...
> > tools/testing/selftests/powerpc/pmu/ebb/fork_cleanup_test.c | 1 -
>
> I took this hunk via the powerpc tree.
How about taking this entirely through a single tree ?
or Shall I send these changes in different patches ?
>
> > diff --git a/tools/testing/selftests/powerpc/pmu/ebb/fork_cleanup_test.c b/tools/testing/selftests/powerpc/pmu/ebb/fork_cleanup_test.c
> > index 167135b..af1b802 100644
> > --- a/tools/testing/selftests/powerpc/pmu/ebb/fork_cleanup_test.c
> > +++ b/tools/testing/selftests/powerpc/pmu/ebb/fork_cleanup_test.c
> > @@ -11,7 +11,6 @@
> > #include <sys/wait.h>
> > #include <unistd.h>
> > #include <setjmp.h>
> > -#include <signal.h>
> >
> > #include "ebb.h"
>
>
> cheers
^ permalink raw reply
* Re: [PATCH net-next v2 5/7] net: sched: pie: add more cases to auto-tune alpha and beta
From: kbuild test robot @ 2019-02-26 5:31 UTC (permalink / raw)
To: Leslie Monis
Cc: kbuild-all, jhs, netdev, dave, Mohit P. Tahiliani, Dhaval Khandla,
Hrishikesh Hiraskar, Manish Kumar B, Sachin D . Patil,
Leslie Monis
In-Reply-To: <20190225102051.12268-6-lesliemonis@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]
Hi Mohit,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Leslie-Monis/net-sched-pie-align-PIE-implementation-with-RFC-8033/20190226-041701
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=arm
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: net/sched/sch_pie.o: in function `pie_timer':
>> sch_pie.c:(.text+0xc3c): undefined reference to `__aeabi_uldivmod'
>> arm-linux-gnueabi-ld: sch_pie.c:(.text+0xed8): undefined reference to `__aeabi_uldivmod'
arm-linux-gnueabi-ld: net/sched/sch_pie.o: in function `pie_qdisc_enqueue':
sch_pie.c:(.text+0x168c): undefined reference to `__aeabi_uldivmod'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67762 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: Martin Lau @ 2019-02-26 5:37 UTC (permalink / raw)
To: Lorenz Bauer
Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org,
linux-api@vger.kernel.org
In-Reply-To: <CACAyw9_smq+c8rUt3ONk+J28CzPefuEOgs8at3q9_5s3zN0_1w@mail.gmail.com>
On Mon, Feb 25, 2019 at 06:26:42PM +0000, Lorenz Bauer wrote:
> On Sat, 23 Feb 2019 at 00:44, Martin Lau <kafai@fb.com> wrote:
> >
> > On Fri, Feb 22, 2019 at 09:50:55AM +0000, Lorenz Bauer wrote:
> > > Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
> > > to a known connection. However, there is one corner case: no sockets are
> > > created if SYN cookies are active. This means that the final ACK in the
> > > 3WHS is misclassified.
> > >
> > > Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
> > > and then check whether a packet is a valid SYN cookie ACK.
> > >
> > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > > ---
> > > include/uapi/linux/bpf.h | 18 ++++++++++-
> > > net/core/filter.c | 68 ++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 85 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index bcdd2474eee7..bc2af87e9621 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -2359,6 +2359,21 @@ union bpf_attr {
> > > * Return
> > > * A **struct bpf_tcp_sock** pointer on success, or NULL in
> > > * case of failure.
> > > + *
> > > + * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> > > + * Description
> > > + * Check whether iph and th contain a valid SYN cookie ACK for
> > > + * the listening socket in sk.
> > > + *
> > > + * iph points to the start of the IPv4 or IPv6 header, while
> > > + * iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
> > > + *
> > > + * th points to the start of the TCP header, while th_len contains
> > > + * sizeof(struct tcphdr).
> > > + *
> > > + * Return
> > > + * 0 if iph and th are a valid SYN cookie ACK, or a negative error
> > > + * otherwise.
> > > */
> > > #define __BPF_FUNC_MAPPER(FN) \
> > > FN(unspec), \
> > > @@ -2457,7 +2472,8 @@ union bpf_attr {
> > > FN(spin_lock), \
> > > FN(spin_unlock), \
> > > FN(sk_fullsock), \
> > > - FN(tcp_sock),
> > > + FN(tcp_sock), \
> > > + FN(sk_check_syncookie),
> > >
> > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > * function eBPF program intends to call
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index 85749f6ec789..9e68897cc7ed 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
> > > .arg1_type = ARG_PTR_TO_SOCK_COMMON,
> > > };
> > >
> > > +BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> > s/bpf_sk_check_syncookie/bpf_tcp_check_syncookie/>
> >
> > > + struct tcphdr *, th, u32, th_len)
> > > +{
> > > +#if IS_ENABLED(CONFIG_SYN_COOKIES)
> > nit. "#ifdef CONFIG_SYN_COOKIES" such that it is clear it is a bool kconfig.
> >
> > > + u32 cookie;
> > > + int ret;
> > > +
> > > + if (unlikely(th_len < sizeof(*th)))
> > > + return -EINVAL;
> > > +
> > > + /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> > > + if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
> > From the test program in patch 3, the "sk" here is obtained from
> > bpf_sk_lookup_tcp() which does a sk_to_full_sk() before returning.
> > AFAICT, meaning bpf_sk_lookup_tcp() will return the listening sk
> > even if there is a request_sock. Does it make sense to check
> > syncookie if there is already a request_sock?
>
> No, that doesn't make a lot of sense. I hadn't realised that
> sk_lookup_tcp only returns full sockets.
> This means we need a way to detect that there is a request sock for a
> given tuple.
>
> * adding a reqsk_exists(tuple) helper means we have to pay the lookup cost twice
> * drop the sk argument and do the necessary lookups in the helper
> itself, but that also
> wastes a call to __inet_lookup_listener
> * skip sk_to_full_sk() in a helper and return RET_PTR_TO_SOCK_COMMON,
> but that violates a bunch of assumptions (e.g. calling bpf_sk_release on them)
How about creating a new lookup helper, bpf_sk"c"_lookup_tcp,
that does not call sk_to_full_sk() before returning.
Its ".ret_type" will be RET_PTR_TO_SOCK_COMMON_OR_NULL which its
reference(-counting) state has to be tracked in the verifier also.
Mainly in check_helper_call(), iirc.
The bpf_prog can then check bpf_sock->state for TCP_LISTEN,
call bpf_tcp_sock() to get the TCP listener sock and pass to
the bpf_tcp_check_syncookie()
>
> For context: ultimately we want use this to answer the question: does
> this (encapsulated)
> packet contain a payload destined to a local socket? Amongst the edge
> cases we need to
> handle are ICMP Packet Too Big messages and SYN cookies. A solution
> would be to hide
> all this in an "uber" helper that takes pointers to the L3 / L4
> headers and returns a verdict,
> but that seems a bit gross.
Please include this use case in the commit message.
It is useful.
>
> >
> > > + return -EINVAL;
> > > +
> > > + if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
> > Should tcp_synq_no_recent_overflow(tp) be checked also?
> >
>
> Yes, not sure how that slipped out.
>
> > > + return -EINVAL;
> > > +
> > > + if (!th->ack || th->rst)
> > How about th->syn?
> >
>
> Yes, I missed the fact that the callers in tcp_ipv{4,6}.c check this.
>
> > > + return -ENOENT;
> > > +
> > > + cookie = ntohl(th->ack_seq) - 1;
> > > +
> > > + switch (sk->sk_family) {
> > > + case AF_INET:
> > > + if (unlikely(iph_len < sizeof(struct iphdr)))
> > > + return -EINVAL;
> > > +
> > > + ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
> > > + break;
> > > +
> > > +#if IS_ENABLED(CONFIG_IPV6)
> > > + case AF_INET6:
> > > + if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> > > + return -EINVAL;
> > > +
> > > + ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
> > > + break;
> > > +#endif /* CONFIG_IPV6 */
> > > +
> > > + default:
> > > + return -EPROTONOSUPPORT;
> > > + }
> > > +
> > > + if (ret > 0)
> > > + return 0;
> > > +
> > > + return -ENOENT;
> > > +#else
> > > + return -ENOTSUP;
> > > +#endif
> > > +}
> > > +
> > > +static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
> > > + .func = bpf_sk_check_syncookie,
> > > + .gpl_only = true,
> > > + .pkt_access = true,
> > > + .ret_type = RET_INTEGER,
> > > + .arg1_type = ARG_PTR_TO_SOCKET,
> > I think it should be ARG_PTR_TO_TCP_SOCK
> >
> > > + .arg2_type = ARG_PTR_TO_MEM,
> > > + .arg3_type = ARG_CONST_SIZE,
> > > + .arg4_type = ARG_PTR_TO_MEM,
> > > + .arg5_type = ARG_CONST_SIZE,
> > > +};
> > > +
> > > #endif /* CONFIG_INET */
>
>
>
> --
> Lorenz Bauer | Systems Engineer
> 25 Lavington St., London SE1 0NZ
>
> https://urldefense.proofpoint.com/v2/url?u=http-3A__www.cloudflare.com&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=xhDwvX3iD-mbqSrx-L8XQNaZiYFZzMWNo_2Y38Z9j34&s=I4Ag3HflabFppFv7UtMp8WnMVSqCDW0W28ziWIvuwDE&e=
^ permalink raw reply
* [PATCH net-next 0/2] vxlan: create and changelink extack support
From: Roopa Prabhu @ 2019-02-26 6:03 UTC (permalink / raw)
To: davem; +Cc: netdev, dsa, sd, johannes
From: Roopa Prabhu <roopa@cumulusnetworks.com>
This series adds extack support to changelink paths.
In the process re-factors flag sets to a separate helper.
Also adds some changelink testcases to rtnetlink.sh
(This series was initially part of another series that
tried to support changelink for more attributes.
But after some feedback from sabrina, i have dropped the
'support changelink for more attributes' part because some
of them cannot be supported today or may require additional
use-case handling code. These can be done separately
as and when we see the need for it.)
Roopa Prabhu (2):
vxlan: add extack support for create and changelink
tools: selftests: rtnetlink: add testcases for vxlan flag sets
drivers/net/vxlan.c | 208 +++++++++++++++++++++----------
include/net/vxlan.h | 31 +++++
tools/testing/selftests/net/rtnetlink.sh | 52 ++++++++
3 files changed, 224 insertions(+), 67 deletions(-)
--
2.1.4
^ permalink raw reply
* [PATCH net-next 1/2] vxlan: add extack support for create and changelink
From: Roopa Prabhu @ 2019-02-26 6:03 UTC (permalink / raw)
To: davem; +Cc: netdev, dsa, sd, johannes
In-Reply-To: <1551160982-32685-1-git-send-email-roopa@cumulusnetworks.com>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
This patch adds extack coverage in vxlan link
create and changelink paths. Introduces a new helper
vxlan_nl2flags to consolidate flag attribute validation.
thanks to Johannes Berg for some tips to construct the
generic vxlan flag extack strings.
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
drivers/net/vxlan.c | 208 +++++++++++++++++++++++++++++++++++-----------------
include/net/vxlan.h | 31 ++++++++
2 files changed, 172 insertions(+), 67 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 577201c..a3c46d7 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3583,11 +3583,40 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
return err;
}
+/* Set/clear flags based on attribute */
+static int vxlan_nl2flag(struct vxlan_config *conf, struct nlattr *tb[],
+ int attrtype, unsigned long mask, bool changelink,
+ bool changelink_supported,
+ struct netlink_ext_ack *extack)
+{
+ unsigned long flags;
+
+ if (!tb[attrtype])
+ return 0;
+
+ if (changelink && !changelink_supported) {
+ vxlan_flag_attr_error(attrtype, extack);
+ return -EOPNOTSUPP;
+ }
+
+ if (vxlan_policy[attrtype].type == NLA_FLAG)
+ flags = conf->flags | mask;
+ else if (nla_get_u8(tb[attrtype]))
+ flags = conf->flags | mask;
+ else
+ flags = conf->flags & ~mask;
+
+ conf->flags = flags;
+
+ return 0;
+}
+
static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
struct net_device *dev, struct vxlan_config *conf,
- bool changelink)
+ bool changelink, struct netlink_ext_ack *extack)
{
struct vxlan_dev *vxlan = netdev_priv(dev);
+ int err = 0;
memset(conf, 0, sizeof(*conf));
@@ -3598,40 +3627,54 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
if (data[IFLA_VXLAN_ID]) {
__be32 vni = cpu_to_be32(nla_get_u32(data[IFLA_VXLAN_ID]));
- if (changelink && (vni != conf->vni))
+ if (changelink && (vni != conf->vni)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_ID], "Cannot change VNI");
return -EOPNOTSUPP;
+ }
conf->vni = cpu_to_be32(nla_get_u32(data[IFLA_VXLAN_ID]));
}
if (data[IFLA_VXLAN_GROUP]) {
- if (changelink && (conf->remote_ip.sa.sa_family != AF_INET))
+ if (changelink && (conf->remote_ip.sa.sa_family != AF_INET)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP], "New group address family does not match old group");
return -EOPNOTSUPP;
+ }
conf->remote_ip.sin.sin_addr.s_addr = nla_get_in_addr(data[IFLA_VXLAN_GROUP]);
conf->remote_ip.sa.sa_family = AF_INET;
} else if (data[IFLA_VXLAN_GROUP6]) {
- if (!IS_ENABLED(CONFIG_IPV6))
+ if (!IS_ENABLED(CONFIG_IPV6)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP6], "IPv6 support not enabled in the kernel");
return -EPFNOSUPPORT;
+ }
- if (changelink && (conf->remote_ip.sa.sa_family != AF_INET6))
+ if (changelink && (conf->remote_ip.sa.sa_family != AF_INET6)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP6], "New group address family does not match old group");
return -EOPNOTSUPP;
+ }
conf->remote_ip.sin6.sin6_addr = nla_get_in6_addr(data[IFLA_VXLAN_GROUP6]);
conf->remote_ip.sa.sa_family = AF_INET6;
}
if (data[IFLA_VXLAN_LOCAL]) {
- if (changelink && (conf->saddr.sa.sa_family != AF_INET))
+ if (changelink && (conf->saddr.sa.sa_family != AF_INET)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL], "New local address family does not match old");
return -EOPNOTSUPP;
+ }
conf->saddr.sin.sin_addr.s_addr = nla_get_in_addr(data[IFLA_VXLAN_LOCAL]);
conf->saddr.sa.sa_family = AF_INET;
} else if (data[IFLA_VXLAN_LOCAL6]) {
- if (!IS_ENABLED(CONFIG_IPV6))
+ if (!IS_ENABLED(CONFIG_IPV6)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL6], "IPv6 support not enabled in the kernel");
return -EPFNOSUPPORT;
+ }
- if (changelink && (conf->saddr.sa.sa_family != AF_INET6))
+ if (changelink && (conf->saddr.sa.sa_family != AF_INET6)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL6], "New local address family does not match old");
return -EOPNOTSUPP;
+ }
/* TODO: respect scope id */
conf->saddr.sin6.sin6_addr = nla_get_in6_addr(data[IFLA_VXLAN_LOCAL6]);
@@ -3648,9 +3691,12 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]);
if (data[IFLA_VXLAN_TTL_INHERIT]) {
- if (changelink)
- return -EOPNOTSUPP;
- conf->flags |= VXLAN_F_TTL_INHERIT;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_TTL_INHERIT,
+ VXLAN_F_TTL_INHERIT, changelink, false,
+ extack);
+ if (err)
+ return err;
+
}
if (data[IFLA_VXLAN_LABEL])
@@ -3658,10 +3704,11 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
IPV6_FLOWLABEL_MASK;
if (data[IFLA_VXLAN_LEARNING]) {
- if (nla_get_u8(data[IFLA_VXLAN_LEARNING]))
- conf->flags |= VXLAN_F_LEARN;
- else
- conf->flags &= ~VXLAN_F_LEARN;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_LEARNING,
+ VXLAN_F_LEARN, changelink, true,
+ extack);
+ if (err)
+ return err;
} else if (!changelink) {
/* default to learn on a new device */
conf->flags |= VXLAN_F_LEARN;
@@ -3671,44 +3718,52 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
conf->age_interval = nla_get_u32(data[IFLA_VXLAN_AGEING]);
if (data[IFLA_VXLAN_PROXY]) {
- if (changelink)
- return -EOPNOTSUPP;
- if (nla_get_u8(data[IFLA_VXLAN_PROXY]))
- conf->flags |= VXLAN_F_PROXY;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_PROXY,
+ VXLAN_F_PROXY, changelink, false,
+ extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_RSC]) {
- if (changelink)
- return -EOPNOTSUPP;
- if (nla_get_u8(data[IFLA_VXLAN_RSC]))
- conf->flags |= VXLAN_F_RSC;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_RSC,
+ VXLAN_F_RSC, changelink, false,
+ extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_L2MISS]) {
- if (changelink)
- return -EOPNOTSUPP;
- if (nla_get_u8(data[IFLA_VXLAN_L2MISS]))
- conf->flags |= VXLAN_F_L2MISS;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_L2MISS,
+ VXLAN_F_L2MISS, changelink, false,
+ extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_L3MISS]) {
- if (changelink)
- return -EOPNOTSUPP;
- if (nla_get_u8(data[IFLA_VXLAN_L3MISS]))
- conf->flags |= VXLAN_F_L3MISS;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_L3MISS,
+ VXLAN_F_L3MISS, changelink, false,
+ extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_LIMIT]) {
- if (changelink)
+ if (changelink) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LIMIT],
+ "Cannot change limit");
return -EOPNOTSUPP;
+ }
conf->addrmax = nla_get_u32(data[IFLA_VXLAN_LIMIT]);
}
if (data[IFLA_VXLAN_COLLECT_METADATA]) {
- if (changelink)
- return -EOPNOTSUPP;
- if (nla_get_u8(data[IFLA_VXLAN_COLLECT_METADATA]))
- conf->flags |= VXLAN_F_COLLECT_METADATA;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_COLLECT_METADATA,
+ VXLAN_F_COLLECT_METADATA, changelink, false,
+ extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_PORT_RANGE]) {
@@ -3718,72 +3773,92 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
conf->port_min = ntohs(p->low);
conf->port_max = ntohs(p->high);
} else {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_PORT_RANGE],
+ "Cannot change port range");
return -EOPNOTSUPP;
}
}
if (data[IFLA_VXLAN_PORT]) {
- if (changelink)
+ if (changelink) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_PORT],
+ "Cannot change port");
return -EOPNOTSUPP;
+ }
conf->dst_port = nla_get_be16(data[IFLA_VXLAN_PORT]);
}
if (data[IFLA_VXLAN_UDP_CSUM]) {
- if (changelink)
+ if (changelink) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_UDP_CSUM],
+ "Cannot change UDP_CSUM flag");
return -EOPNOTSUPP;
+ }
if (!nla_get_u8(data[IFLA_VXLAN_UDP_CSUM]))
conf->flags |= VXLAN_F_UDP_ZERO_CSUM_TX;
}
if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) {
- if (changelink)
- return -EOPNOTSUPP;
- if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]))
- conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_TX;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_UDP_ZERO_CSUM6_TX,
+ VXLAN_F_UDP_ZERO_CSUM6_TX, changelink,
+ false, extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]) {
- if (changelink)
- return -EOPNOTSUPP;
- if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]))
- conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_RX;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_UDP_ZERO_CSUM6_RX,
+ VXLAN_F_UDP_ZERO_CSUM6_RX, changelink,
+ false, extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_REMCSUM_TX]) {
- if (changelink)
- return -EOPNOTSUPP;
- if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_TX]))
- conf->flags |= VXLAN_F_REMCSUM_TX;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_TX,
+ VXLAN_F_REMCSUM_TX, changelink, false,
+ extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_REMCSUM_RX]) {
- if (changelink)
- return -EOPNOTSUPP;
- if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_RX]))
- conf->flags |= VXLAN_F_REMCSUM_RX;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_RX,
+ VXLAN_F_REMCSUM_RX, changelink, false,
+ extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_GBP]) {
- if (changelink)
- return -EOPNOTSUPP;
- conf->flags |= VXLAN_F_GBP;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_GBP,
+ VXLAN_F_GBP, changelink, false, extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_GPE]) {
- if (changelink)
- return -EOPNOTSUPP;
- conf->flags |= VXLAN_F_GPE;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_GPE,
+ VXLAN_F_GPE, changelink, false,
+ extack);
+ if (err)
+ return err;
}
if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
- if (changelink)
- return -EOPNOTSUPP;
- conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
+ err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_NOPARTIAL,
+ VXLAN_F_REMCSUM_NOPARTIAL, changelink,
+ false, extack);
+ if (err)
+ return err;
}
if (tb[IFLA_MTU]) {
- if (changelink)
+ if (changelink) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_MTU],
+ "Cannot change mtu");
return -EOPNOTSUPP;
+ }
conf->mtu = nla_get_u32(tb[IFLA_MTU]);
}
@@ -3800,7 +3875,7 @@ static int vxlan_newlink(struct net *src_net, struct net_device *dev,
struct vxlan_config conf;
int err;
- err = vxlan_nl2conf(tb, data, dev, &conf, false);
+ err = vxlan_nl2conf(tb, data, dev, &conf, false, extack);
if (err)
return err;
@@ -3817,8 +3892,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
struct vxlan_config conf;
int err;
- err = vxlan_nl2conf(tb, data,
- dev, &conf, true);
+ err = vxlan_nl2conf(tb, data, dev, &conf, true, extack);
if (err)
return err;
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 0976781..00254a5 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -453,4 +453,35 @@ vxlan_fdb_clear_offload(const struct net_device *dev, __be32 vni)
}
#endif
+static inline void vxlan_flag_attr_error(int attrtype,
+ struct netlink_ext_ack *extack)
+{
+#define VXLAN_FLAG(flg) \
+ case IFLA_VXLAN_##flg: \
+ NL_SET_ERR_MSG_MOD(extack, \
+ "cannot change " #flg " flag"); \
+ break
+ switch (attrtype) {
+ VXLAN_FLAG(TTL_INHERIT);
+ VXLAN_FLAG(LEARNING);
+ VXLAN_FLAG(PROXY);
+ VXLAN_FLAG(RSC);
+ VXLAN_FLAG(L2MISS);
+ VXLAN_FLAG(L3MISS);
+ VXLAN_FLAG(COLLECT_METADATA);
+ VXLAN_FLAG(UDP_ZERO_CSUM6_TX);
+ VXLAN_FLAG(UDP_ZERO_CSUM6_RX);
+ VXLAN_FLAG(REMCSUM_TX);
+ VXLAN_FLAG(REMCSUM_RX);
+ VXLAN_FLAG(GBP);
+ VXLAN_FLAG(GPE);
+ VXLAN_FLAG(REMCSUM_NOPARTIAL);
+ default:
+ NL_SET_ERR_MSG_MOD(extack, \
+ "cannot change flag");
+ break;
+ }
+#undef VXLAN_FLAG
+}
+
#endif
--
2.1.4
^ permalink raw reply related
* [PATCH net-next 2/2] tools: selftests: rtnetlink: add testcases for vxlan flag sets
From: Roopa Prabhu @ 2019-02-26 6:03 UTC (permalink / raw)
To: davem; +Cc: netdev, dsa, sd, johannes
In-Reply-To: <1551160982-32685-1-git-send-email-roopa@cumulusnetworks.com>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
This patch extends rtnetlink.sh to cover some vxlan flag
netlink attribute sets.
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
tools/testing/selftests/net/rtnetlink.sh | 52 ++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index 78fc593..5baf35a 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -408,6 +408,58 @@ kci_test_encap_vxlan()
ip netns exec "$testns" ip link add link "$vxlan" name "$vlan" type vlan id 1
check_err $?
+ # changelink testcases
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan vni 43 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan group ffe5::5 dev "$devdummy" 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan ttl inherit 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan ttl 64
+ check_err $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan nolearning
+ check_err $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan proxy 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan norsc 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan l2miss 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan l3miss 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan external 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan udpcsum 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan udp6zerocsumtx 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan udp6zerocsumrx 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan remcsumtx 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan remcsumrx 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan gbp 2>/dev/null
+ check_fail $?
+
+ ip netns exec "$testns" ip link set dev "$vxlan" type vxlan gpe 2>/dev/null
+ check_fail $?
+
ip netns exec "$testns" ip link del "$vxlan"
check_err $?
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 0/2] bpf: context casting for tail call and gtrace prog type
From: Alexei Starovoitov @ 2019-02-26 6:18 UTC (permalink / raw)
To: Kris Van Hees; +Cc: netdev, bpf, daniel
In-Reply-To: <201902251554.x1PFsD3w017626@userv0121.oracle.com>
On Mon, Feb 25, 2019 at 07:54:13AM -0800, Kris Van Hees wrote:
>
> The goal is to further extend the BPF_PROG_TYPE_GTRACE implementation to
> support what tracers commonly need, and I am also looking at ways to further
> extend this model to allow more tracer-specific features as well without the
> need for adding a BPF program types for every tracer.
It seems by themselves the patches don't provide any new functionality,
but instead look like plumbing to call external code.
This is no-go.
There were several attempts to do so in the past, so we documented it here:
Documentation/bpf/bpf_design_QA.rst
Q: New functionality via kernel modules?
----------------------------------------
Q: Can BPF functionality such as new program or map types, new
helpers, etc be added out of kernel module code?
A: NO.
The answer is still the same.
^ permalink raw reply
* Re: [RFC] nasty corner case in unix_dgram_sendmsg()
From: Al Viro @ 2019-02-26 6:28 UTC (permalink / raw)
To: Rainer Weikusat, Jason Baron; +Cc: netdev
In-Reply-To: <20190225035121.GH2217@ZenIV.linux.org.uk>
On Mon, Feb 25, 2019 at 03:51:21AM +0000, Al Viro wrote:
> PS: unix_dgram_sendmsg() is really much too subtle for its own good -
> AFAICS, it *does* avoid blocking operations under sk->lock, but proof
> is considerably more complex than one would like it to be...
Several questions about the whole peer_wake mechanism:
1) why do we even bother with that for SOCK_SEQPACKET? AFAICS, there
we do *not* hit unix_wait_for_peer() on send - it's conditional upon
if (other != sk &&
unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
after we'd checked that other is non-NULL and not dead. For seqpacket
we start with both ends of connection having unix_peer() pointing to
each other and only changed to NULL when one of the ends gets closed.
The socket we'd passed to sendmsg is obviously not dead yet, and we'd
verified that other isn't dead either. So unix_peer(other) must be
equal to sk and we don't go into that if.
In unix_dgram_poll() we might get to unix_dgram_peer_wake_me() for
seqpacket, but only in case when other is dead. In that case
unix_dgram_peer_wake_me() will insert our peer_wake into queue,
see SOCK_DEAD, remove peer_wake from queue and return false.
AFAICS, that serves no purpose whatsoever...
2) the logics in sendmsg is very odd:
* if we'd detected n:1 send *and* found that we need to
wait, do so (not using the peer_wake - other's peer_wait is not
going away). No questions there.
* if we'd detected n:1 send *and* found that we need to
wait, but don't have any remaining timeout, we lock both ends
and check if unix_peer(sk) is (now) not equal to other, either
because it never had or because we'd been hit by connect(2) while
we'd been doing the locking. In that case we fail with -EAGAIN.
Fair enough, but
* if after relocking we see that unix_peer(sk) now
is equal to other, we arrange for wakeup forwarding from other's
peer_wait *and* if that has (likely) succeeded we fail with -EAGAIN.
Huh? What's the point? The only thing that depends upon that
wakeup forwarding is poll, and _that_ will set the forwarding up
on its own.
* if we'd failed (either because other is dead now or
because its queue is not full), we go back to restart_locked.
If it's dead, we'll sod off with ECONNREFUSED, if it's not
full anymore, we'll send the damn thing.
All of that comes at the cost of considerable headache in
unix_dgram_sendmsg() - flag-conditional locking, etc. Why do
we bother? What's wrong with simple "n:1 case detected, queue
full, no timeout left, return -EAGAIN and be done with that"?
IDGI... Am I missing something subtle going on here?
I understand what peer_wake is for, and the poll side of things
is fine; it's sendmsg one that looks weird. What's going on
there?
^ permalink raw reply
* Re: [RFC] nasty corner case in unix_dgram_sendmsg()
From: Al Viro @ 2019-02-26 6:38 UTC (permalink / raw)
To: Rainer Weikusat, Jason Baron; +Cc: netdev
In-Reply-To: <20190226062817.GA17962@ZenIV.linux.org.uk>
On Tue, Feb 26, 2019 at 06:28:17AM +0000, Al Viro wrote:
> 2) the logics in sendmsg is very odd:
> * if we'd detected n:1 send *and* found that we need to
> wait, do so (not using the peer_wake - other's peer_wait is not
> going away). No questions there.
> * if we'd detected n:1 send *and* found that we need to
> wait, but don't have any remaining timeout, we lock both ends
> and check if unix_peer(sk) is (now) not equal to other, either
> because it never had or because we'd been hit by connect(2) while
> we'd been doing the locking. In that case we fail with -EAGAIN.
> Fair enough, but
> * if after relocking we see that unix_peer(sk) now
> is equal to other, we arrange for wakeup forwarding from other's
> peer_wait *and* if that has (likely) succeeded we fail with -EAGAIN.
> Huh? What's the point? The only thing that depends upon that
> wakeup forwarding is poll, and _that_ will set the forwarding up
> on its own.
> * if we'd failed (either because other is dead now or
> because its queue is not full), we go back to restart_locked.
> If it's dead, we'll sod off with ECONNREFUSED, if it's not
> full anymore, we'll send the damn thing.
>
> All of that comes at the cost of considerable headache in
> unix_dgram_sendmsg() - flag-conditional locking, etc. Why do
> we bother? What's wrong with simple "n:1 case detected, queue
> full, no timeout left, return -EAGAIN and be done with that"?
>
> IDGI... Am I missing something subtle going on here?
>
> I understand what peer_wake is for, and the poll side of things
> is fine; it's sendmsg one that looks weird. What's going on
> there?
What I have in mind is something like this (for that part of the
issues):
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 74d1eed7cbd4..85d72ea79924 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1635,7 +1635,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
long timeo;
struct scm_cookie scm;
int data_len = 0;
- int sk_locked;
wait_for_unix_gc();
err = scm_send(sock, msg, &scm, false);
@@ -1713,9 +1712,8 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
goto out_free;
}
- sk_locked = 0;
unix_state_lock(other);
-restart_locked:
+
err = -EPERM;
if (!unix_may_send(sk, other))
goto out_unlock;
@@ -1728,8 +1726,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
unix_state_unlock(other);
sock_put(other);
- if (!sk_locked)
- unix_state_lock(sk);
+ unix_state_lock(sk);
err = 0;
if (unix_peer(sk) == other) {
@@ -1767,36 +1764,19 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
*/
if (other != sk &&
unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
- if (timeo) {
- timeo = unix_wait_for_peer(other, timeo);
-
- err = sock_intr_errno(timeo);
- if (signal_pending(current))
- goto out_free;
-
- goto restart;
- }
-
- if (!sk_locked) {
- unix_state_unlock(other);
- unix_state_double_lock(sk, other);
- }
-
- if (unix_peer(sk) != other ||
- unix_dgram_peer_wake_me(sk, other)) {
+ if (!timeo) {
err = -EAGAIN;
- sk_locked = 1;
goto out_unlock;
}
- if (!sk_locked) {
- sk_locked = 1;
- goto restart_locked;
- }
- }
+ timeo = unix_wait_for_peer(other, timeo);
- if (unlikely(sk_locked))
- unix_state_unlock(sk);
+ err = sock_intr_errno(timeo);
+ if (signal_pending(current))
+ goto out_free;
+
+ goto restart;
+ }
if (sock_flag(other, SOCK_RCVTSTAMP))
__net_timestamp(skb);
@@ -1809,8 +1789,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
return len;
out_unlock:
- if (sk_locked)
- unix_state_unlock(sk);
unix_state_unlock(other);
out_free:
kfree_skb(skb);
^ permalink raw reply related
* [BUG] net/sched : qlen can not really be per cpu ?
From: Eric Dumazet @ 2019-02-26 6:42 UTC (permalink / raw)
To: John Fastabend, Networking, Jamal Hadi Salim, Cong Wang,
Jiri Pirko
HTB + pfifo_fast as a leaf qdisc hits badly the following warning in htb_activate() :
WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
This is because pfifo_fast does not update sch->q.qlen, but per cpu counters.
So cl->leaf.q->q.qlen is zero.
HFSC, CBQ, DRR, QFQ have the same problem.
Any ideas how we can fix this ?
Thanks !
^ permalink raw reply
* Re: [PATCH 0/2] bpf: context casting for tail call and gtrace prog type
From: Kris Van Hees @ 2019-02-26 6:46 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: Kris Van Hees, netdev, bpf, daniel
In-Reply-To: <20190226061823.f7hfs7yojfuycqmg@ast-mbp.dhcp.thefacebook.com>
On Mon, Feb 25, 2019 at 10:18:25PM -0800, Alexei Starovoitov wrote:
> On Mon, Feb 25, 2019 at 07:54:13AM -0800, Kris Van Hees wrote:
> >
> > The goal is to further extend the BPF_PROG_TYPE_GTRACE implementation to
> > support what tracers commonly need, and I am also looking at ways to further
> > extend this model to allow more tracer-specific features as well without the
> > need for adding a BPF program types for every tracer.
>
> It seems by themselves the patches don't provide any new functionality,
> but instead look like plumbing to call external code.
The patches are definitely not plumbing to call external code, and if I gave
that impression I apologise. I overlooked the information you quote below on
allowing new functionality through modules when I wrote the comment above but
please note that it was a forward-looking comment in terms of what could be
done - not a reason for the patches that I submitted.
The patches accomplish something that is totally independent from that: they
make it possible for existing events that execute BPF programs when triggered
to transfer control to a BPF program with a more rich context. The first
patch makes such a transfer possible (using tail-call combined with converting
the context to the new program type), and the second patch provides one such
program type (generic trace). The new functionality provided by the program
type is direct access to task information that previously could only be
obtained through helper calls. E.g. the new program type allows programs to
access the task state, prio, ppid, euid, and egid. None of those pieces of
information can currently be obtained unless you start poking around in
memory using bpf_probe_read() helper calls.
> This is no-go.
> There were several attempts to do so in the past, so we documented it here:
> Documentation/bpf/bpf_design_QA.rst
> Q: New functionality via kernel modules?
> ----------------------------------------
> Q: Can BPF functionality such as new program or map types, new
> helpers, etc be added out of kernel module code?
>
> A: NO.
>
> The answer is still the same.
Thanks for pointing this out - but again, my reference to modules was merely
musing about the possibilities. This information clearly closes the door on
that train of thought, but that is not directly related to what I am doing
with the patches I submitted.
Kris
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox