* [PATCH v4 1/5] net: macb: add phylink support
From: Parshuram Thombare @ 2019-06-23 9:17 UTC (permalink / raw)
To: andrew, nicolas.ferre, davem, f.fainelli
Cc: linux, netdev, hkallweit1, linux-kernel, rafalc, aniljoy, piotrs,
pthombar
In-Reply-To: <1561281419-6030-1-git-send-email-pthombar@cadence.com>
This patch replace phylib API's by phylink API's.
Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
---
drivers/net/ethernet/cadence/Kconfig | 2 +-
drivers/net/ethernet/cadence/macb.h | 3 +
drivers/net/ethernet/cadence/macb_main.c | 304 ++++++++++++-----------
3 files changed, 166 insertions(+), 143 deletions(-)
diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index 1766697c9c5a..d71411a71587 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -22,7 +22,7 @@ if NET_VENDOR_CADENCE
config MACB
tristate "Cadence MACB/GEM support"
depends on HAS_DMA
- select PHYLIB
+ select PHYLINK
---help---
The Cadence MACB ethernet interface is found on many Atmel AT32 and
AT91 parts. This driver also supports the Cadence GEM (Gigabit
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 6ff123da6a14..8629d345af31 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -11,6 +11,7 @@
#include <linux/ptp_clock_kernel.h>
#include <linux/net_tstamp.h>
#include <linux/interrupt.h>
+#include <linux/phylink.h>
#if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP)
#define MACB_EXT_DESC
@@ -1224,6 +1225,8 @@ struct macb {
u32 rx_intr_mask;
struct macb_pm_data pm_data;
+ struct phylink *pl;
+ struct phylink_config pl_config;
};
#ifdef CONFIG_MACB_USE_HWSTAMP
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b4fa0111cd7a..1d6527a5313f 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -36,6 +36,7 @@
#include <linux/tcp.h>
#include <linux/iopoll.h>
#include <linux/pm_runtime.h>
+#include <linux/phylink.h>
#include "macb.h"
/* This structure is only used for MACB on SiFive FU540 devices */
@@ -435,115 +436,145 @@ static void macb_set_tx_clk(struct clk *clk, int speed, struct net_device *dev)
netdev_err(dev, "adjusting tx_clk failed.\n");
}
-static void macb_handle_link_change(struct net_device *dev)
+static void gem_phylink_validate(struct phylink_config *pl_config,
+ unsigned long *supported,
+ struct phylink_link_state *state)
{
- struct macb *bp = netdev_priv(dev);
- struct phy_device *phydev = dev->phydev;
- unsigned long flags;
- int status_change = 0;
+ struct net_device *netdev = to_net_dev(pl_config->dev);
+ struct macb *bp = netdev_priv(netdev);
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+ switch (state->interface) {
+ case PHY_INTERFACE_MODE_GMII:
+ case PHY_INTERFACE_MODE_RGMII:
+ if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
+ phylink_set(mask, 1000baseT_Full);
+ phylink_set(mask, 1000baseX_Full);
+ if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) {
+ phylink_set(mask, 1000baseT_Half);
+ phylink_set(mask, 1000baseT_Half);
+ }
+ }
+ /* fallthrough */
+ case PHY_INTERFACE_MODE_MII:
+ case PHY_INTERFACE_MODE_RMII:
+ phylink_set(mask, 10baseT_Half);
+ phylink_set(mask, 10baseT_Full);
+ phylink_set(mask, 100baseT_Half);
+ phylink_set(mask, 100baseT_Full);
+ break;
+ default:
+ break;
+ }
- spin_lock_irqsave(&bp->lock, flags);
+ linkmode_and(supported, supported, mask);
+ linkmode_and(state->advertising, state->advertising, mask);
- if (phydev->link) {
- if ((bp->speed != phydev->speed) ||
- (bp->duplex != phydev->duplex)) {
- u32 reg;
+}
- reg = macb_readl(bp, NCFGR);
- reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
- if (macb_is_gem(bp))
- reg &= ~GEM_BIT(GBE);
+static int gem_phylink_mac_link_state(struct phylink_config *pl_config,
+ struct phylink_link_state *state)
+{
+ return -EOPNOTSUPP;
+}
- if (phydev->duplex)
- reg |= MACB_BIT(FD);
- if (phydev->speed == SPEED_100)
- reg |= MACB_BIT(SPD);
- if (phydev->speed == SPEED_1000 &&
- bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
- reg |= GEM_BIT(GBE);
+static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
+ const struct phylink_link_state *state)
+{
+ struct net_device *netdev = to_net_dev(pl_config->dev);
+ struct macb *bp = netdev_priv(netdev);
+ unsigned long flags;
- macb_or_gem_writel(bp, NCFGR, reg);
+ spin_lock_irqsave(&bp->lock, flags);
- bp->speed = phydev->speed;
- bp->duplex = phydev->duplex;
- status_change = 1;
- }
- }
+ if (!phylink_autoneg_inband(mode) &&
+ (bp->speed != state->speed ||
+ bp->duplex != state->duplex)) {
+ u32 reg;
- if (phydev->link != bp->link) {
- if (!phydev->link) {
- bp->speed = 0;
- bp->duplex = -1;
+ reg = macb_readl(bp, NCFGR);
+ reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
+ if (macb_is_gem(bp))
+ reg &= ~GEM_BIT(GBE);
+ if (state->duplex)
+ reg |= MACB_BIT(FD);
+
+ switch (state->speed) {
+ case SPEED_1000:
+ reg |= GEM_BIT(GBE);
+ break;
+ case SPEED_100:
+ reg |= MACB_BIT(SPD);
+ break;
+ default:
+ break;
}
- bp->link = phydev->link;
+ macb_or_gem_writel(bp, NCFGR, reg);
+
+ bp->speed = state->speed;
+ bp->duplex = state->duplex;
- status_change = 1;
+ if (state->link)
+ macb_set_tx_clk(bp->tx_clk, state->speed, netdev);
}
spin_unlock_irqrestore(&bp->lock, flags);
+}
- if (status_change) {
- if (phydev->link) {
- /* Update the TX clock rate if and only if the link is
- * up and there has been a link change.
- */
- macb_set_tx_clk(bp->tx_clk, phydev->speed, dev);
+static void gem_mac_link_up(struct phylink_config *pl_config, unsigned int mode,
+ phy_interface_t interface, struct phy_device *phy)
+{
+ struct net_device *netdev = to_net_dev(pl_config->dev);
+ struct macb *bp = netdev_priv(netdev);
- netif_carrier_on(dev);
- netdev_info(dev, "link up (%d/%s)\n",
- phydev->speed,
- phydev->duplex == DUPLEX_FULL ?
- "Full" : "Half");
- } else {
- netif_carrier_off(dev);
- netdev_info(dev, "link down\n");
- }
- }
+ bp->link = 1;
+ /* Enable TX and RX */
+ macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));
+}
+
+static void gem_mac_link_down(struct phylink_config *pl_config,
+ unsigned int mode, phy_interface_t interface)
+{
+ struct net_device *netdev = to_net_dev(pl_config->dev);
+ struct macb *bp = netdev_priv(netdev);
+
+ bp->link = 0;
+ /* Disable TX and RX */
+ macb_writel(bp, NCR,
+ macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE)));
}
+static const struct phylink_mac_ops gem_phylink_ops = {
+ .validate = gem_phylink_validate,
+ .mac_link_state = gem_phylink_mac_link_state,
+ .mac_config = gem_mac_config,
+ .mac_link_up = gem_mac_link_up,
+ .mac_link_down = gem_mac_link_down,
+};
+
/* based on au1000_eth. c*/
static int macb_mii_probe(struct net_device *dev)
{
struct macb *bp = netdev_priv(dev);
struct phy_device *phydev;
struct device_node *np;
- int ret, i;
+ int ret;
np = bp->pdev->dev.of_node;
ret = 0;
- if (np) {
- if (of_phy_is_fixed_link(np)) {
- bp->phy_node = of_node_get(np);
- } else {
- bp->phy_node = of_parse_phandle(np, "phy-handle", 0);
- /* fallback to standard phy registration if no
- * phy-handle was found nor any phy found during
- * dt phy registration
- */
- if (!bp->phy_node && !phy_find_first(bp->mii_bus)) {
- for (i = 0; i < PHY_MAX_ADDR; i++) {
- phydev = mdiobus_scan(bp->mii_bus, i);
- if (IS_ERR(phydev) &&
- PTR_ERR(phydev) != -ENODEV) {
- ret = PTR_ERR(phydev);
- break;
- }
- }
-
- if (ret)
- return -ENODEV;
- }
- }
+ bp->pl_config.dev = &dev->dev;
+ bp->pl_config.type = PHYLINK_NETDEV;
+ bp->pl = phylink_create(&bp->pl_config, of_fwnode_handle(np),
+ bp->phy_interface, &gem_phylink_ops);
+ if (IS_ERR(bp->pl)) {
+ netdev_err(dev,
+ "error creating PHYLINK: %ld\n", PTR_ERR(bp->pl));
+ return PTR_ERR(bp->pl);
}
- if (bp->phy_node) {
- phydev = of_phy_connect(dev, bp->phy_node,
- &macb_handle_link_change, 0,
- bp->phy_interface);
- if (!phydev)
- return -ENODEV;
- } else {
+ ret = phylink_of_phy_connect(bp->pl, np, 0);
+ if (ret == -ENODEV && bp->mii_bus) {
phydev = phy_find_first(bp->mii_bus);
if (!phydev) {
netdev_err(dev, "no PHY found\n");
@@ -551,29 +582,18 @@ static int macb_mii_probe(struct net_device *dev)
}
/* attach the mac to the phy */
- ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
- bp->phy_interface);
+ ret = phylink_connect_phy(bp->pl, phydev);
if (ret) {
netdev_err(dev, "Could not attach to PHY\n");
return ret;
}
}
- /* mask with MAC supported features */
- if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
- phy_set_max_speed(phydev, SPEED_1000);
- else
- phy_set_max_speed(phydev, SPEED_100);
-
- if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
- phy_remove_link_mode(phydev,
- ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-
bp->link = 0;
bp->speed = 0;
bp->duplex = -1;
- return 0;
+ return ret;
}
static int macb_mii_init(struct macb *bp)
@@ -601,17 +621,7 @@ static int macb_mii_init(struct macb *bp)
dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
np = bp->pdev->dev.of_node;
- if (np && of_phy_is_fixed_link(np)) {
- if (of_phy_register_fixed_link(np) < 0) {
- dev_err(&bp->pdev->dev,
- "broken fixed-link specification %pOF\n", np);
- goto err_out_free_mdiobus;
- }
-
- err = mdiobus_register(bp->mii_bus);
- } else {
- err = of_mdiobus_register(bp->mii_bus, np);
- }
+ err = of_mdiobus_register(bp->mii_bus, np);
if (err)
goto err_out_free_fixed_link;
@@ -627,7 +637,6 @@ static int macb_mii_init(struct macb *bp)
err_out_free_fixed_link:
if (np && of_phy_is_fixed_link(np))
of_phy_deregister_fixed_link(np);
-err_out_free_mdiobus:
of_node_put(bp->phy_node);
mdiobus_free(bp->mii_bus);
err_out:
@@ -2418,12 +2427,6 @@ static int macb_open(struct net_device *dev)
/* carrier starts down */
netif_carrier_off(dev);
- /* if the phy is not yet register, retry later*/
- if (!dev->phydev) {
- err = -EAGAIN;
- goto pm_exit;
- }
-
/* RX buffers initialization */
macb_init_rx_buffer_size(bp, bufsz);
@@ -2441,7 +2444,7 @@ static int macb_open(struct net_device *dev)
macb_init_hw(bp);
/* schedule a link state check */
- phy_start(dev->phydev);
+ phylink_start(bp->pl);
netif_tx_start_all_queues(dev);
@@ -2468,8 +2471,7 @@ static int macb_close(struct net_device *dev)
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
napi_disable(&queue->napi);
- if (dev->phydev)
- phy_stop(dev->phydev);
+ phylink_stop(bp->pl);
spin_lock_irqsave(&bp->lock, flags);
macb_reset_hw(bp);
@@ -3158,6 +3160,23 @@ static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd)
return ret;
}
+static int gem_ethtool_get_link_ksettings(struct net_device *netdev,
+ struct ethtool_link_ksettings *cmd)
+{
+ struct macb *bp = netdev_priv(netdev);
+
+ return phylink_ethtool_ksettings_get(bp->pl, cmd);
+}
+
+static int
+gem_ethtool_set_link_ksettings(struct net_device *netdev,
+ const struct ethtool_link_ksettings *cmd)
+{
+ struct macb *bp = netdev_priv(netdev);
+
+ return phylink_ethtool_ksettings_set(bp->pl, cmd);
+}
+
static const struct ethtool_ops macb_ethtool_ops = {
.get_regs_len = macb_get_regs_len,
.get_regs = macb_get_regs,
@@ -3165,8 +3184,8 @@ static const struct ethtool_ops macb_ethtool_ops = {
.get_ts_info = ethtool_op_get_ts_info,
.get_wol = macb_get_wol,
.set_wol = macb_set_wol,
- .get_link_ksettings = phy_ethtool_get_link_ksettings,
- .set_link_ksettings = phy_ethtool_set_link_ksettings,
+ .get_link_ksettings = gem_ethtool_get_link_ksettings,
+ .set_link_ksettings = gem_ethtool_set_link_ksettings,
.get_ringparam = macb_get_ringparam,
.set_ringparam = macb_set_ringparam,
};
@@ -3179,8 +3198,8 @@ static const struct ethtool_ops gem_ethtool_ops = {
.get_ethtool_stats = gem_get_ethtool_stats,
.get_strings = gem_get_ethtool_strings,
.get_sset_count = gem_get_sset_count,
- .get_link_ksettings = phy_ethtool_get_link_ksettings,
- .set_link_ksettings = phy_ethtool_set_link_ksettings,
+ .get_link_ksettings = gem_ethtool_get_link_ksettings,
+ .set_link_ksettings = gem_ethtool_set_link_ksettings,
.get_ringparam = macb_get_ringparam,
.set_ringparam = macb_set_ringparam,
.get_rxnfc = gem_get_rxnfc,
@@ -3189,17 +3208,13 @@ static const struct ethtool_ops gem_ethtool_ops = {
static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
{
- struct phy_device *phydev = dev->phydev;
struct macb *bp = netdev_priv(dev);
if (!netif_running(dev))
return -EINVAL;
- if (!phydev)
- return -ENODEV;
-
if (!bp->ptp_info)
- return phy_mii_ioctl(phydev, rq, cmd);
+ return phylink_mii_ioctl(bp->pl, rq, cmd);
switch (cmd) {
case SIOCSHWTSTAMP:
@@ -3207,7 +3222,7 @@ static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
case SIOCGHWTSTAMP:
return bp->ptp_info->get_hwtst(dev, rq);
default:
- return phy_mii_ioctl(phydev, rq, cmd);
+ return phylink_mii_ioctl(bp->pl, rq, cmd);
}
}
@@ -3707,7 +3722,7 @@ static int at91ether_open(struct net_device *dev)
MACB_BIT(HRESP));
/* schedule a link state check */
- phy_start(dev->phydev);
+ phylink_start(lp->pl);
netif_start_queue(dev);
@@ -4180,13 +4195,12 @@ static int macb_probe(struct platform_device *pdev)
struct clk *tsu_clk = NULL;
unsigned int queue_mask, num_queues;
bool native_io;
- struct phy_device *phydev;
struct net_device *dev;
struct resource *regs;
void __iomem *mem;
const char *mac;
struct macb *bp;
- int err, val;
+ int err, val, phy_mode;
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
mem = devm_ioremap_resource(&pdev->dev, regs);
@@ -4307,12 +4321,12 @@ static int macb_probe(struct platform_device *pdev)
macb_get_hwaddr(bp);
}
- err = of_get_phy_mode(np);
- if (err < 0)
+ phy_mode = of_get_phy_mode(np);
+ if (phy_mode < 0)
/* not found in DT, MII by default */
bp->phy_interface = PHY_INTERFACE_MODE_MII;
else
- bp->phy_interface = err;
+ bp->phy_interface = phy_mode;
/* IP specific init */
err = init(pdev);
@@ -4323,8 +4337,6 @@ static int macb_probe(struct platform_device *pdev)
if (err)
goto err_out_free_netdev;
- phydev = dev->phydev;
-
netif_carrier_off(dev);
err = register_netdev(dev);
@@ -4336,8 +4348,6 @@ static int macb_probe(struct platform_device *pdev)
tasklet_init(&bp->hresp_err_tasklet, macb_hresp_error_task,
(unsigned long)bp);
- phy_attached_info(phydev);
-
netdev_info(dev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n",
macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
dev->base_addr, dev->irq, dev->dev_addr);
@@ -4348,7 +4358,9 @@ static int macb_probe(struct platform_device *pdev)
return 0;
err_out_unregister_mdio:
- phy_disconnect(dev->phydev);
+ rtnl_lock();
+ phylink_disconnect_phy(bp->pl);
+ rtnl_unlock();
mdiobus_unregister(bp->mii_bus);
of_node_put(bp->phy_node);
if (np && of_phy_is_fixed_link(np))
@@ -4382,13 +4394,18 @@ static int macb_remove(struct platform_device *pdev)
if (dev) {
bp = netdev_priv(dev);
- if (dev->phydev)
- phy_disconnect(dev->phydev);
+ if (bp->pl) {
+ rtnl_lock();
+ phylink_disconnect_phy(bp->pl);
+ rtnl_unlock();
+ }
mdiobus_unregister(bp->mii_bus);
if (np && of_phy_is_fixed_link(np))
of_phy_deregister_fixed_link(np);
dev->phydev = NULL;
mdiobus_free(bp->mii_bus);
+ if (bp->pl)
+ phylink_destroy(bp->pl);
unregister_netdev(dev);
pm_runtime_disable(&pdev->dev);
@@ -4431,8 +4448,9 @@ static int __maybe_unused macb_suspend(struct device *dev)
for (q = 0, queue = bp->queues; q < bp->num_queues;
++q, ++queue)
napi_disable(&queue->napi);
- phy_stop(netdev->phydev);
- phy_suspend(netdev->phydev);
+ phylink_stop(bp->pl);
+ if (netdev->phydev)
+ phy_suspend(netdev->phydev);
spin_lock_irqsave(&bp->lock, flags);
macb_reset_hw(bp);
spin_unlock_irqrestore(&bp->lock, flags);
@@ -4480,9 +4498,11 @@ static int __maybe_unused macb_resume(struct device *dev)
for (q = 0, queue = bp->queues; q < bp->num_queues;
++q, ++queue)
napi_enable(&queue->napi);
- phy_resume(netdev->phydev);
- phy_init_hw(netdev->phydev);
- phy_start(netdev->phydev);
+ if (netdev->phydev) {
+ phy_resume(netdev->phydev);
+ phy_init_hw(netdev->phydev);
+ }
+ phylink_start(bp->pl);
}
bp->macbgem_ops.mog_init_rings(bp);
--
2.17.1
^ permalink raw reply related
* [PATCH v4 0/5] net: macb: cover letter
From: Parshuram Thombare @ 2019-06-23 9:16 UTC (permalink / raw)
To: andrew, nicolas.ferre, davem, f.fainelli
Cc: linux, netdev, hkallweit1, linux-kernel, rafalc, aniljoy, piotrs,
pthombar
Hello !
This is 4th version of patch set containing following patches
for Cadence ethernet controller driver.
1. 0001-net-macb-add-phylink-support.patch
Replace phylib API's with phylink API's.
2. 0002-net-macb-add-support-for-sgmii-MAC-PHY-interface.patch
This patch add support for SGMII mode.
3. 0004-net-macb-add-support-for-c45-PHY.patch
This patch is to support C45 PHY.
4. 0005-net-macb-add-support-for-high-speed-interface
This patch add support for 10G USXGMII PCS in fixed mode.
5. 0006-net-macb-parameter-added-to-cadence-ethernet-controller-DT-binding
New parameter added to Cadence ethernet controller DT binding
for USXGMII interface.
Changes in v2:
1. Dropped patch configuring TI PHY DP83867 from
Cadence PCI wrapper driver.
2. Removed code registering emulated PHY for fixed mode.
3. Code reformatting as per Andrew's and Florian's suggestions.
Changes in v3:
Based on Russell's suggestions
1. Configure MAC in mac_config only for non in-band modes
2. Handle dynamic phy_mode changes in mac_config
3. Move MAC configurations to mac_config
4. Removed seemingly redundant check for phylink handle
5. Removed code from mac_an_restart and mac_link_state
now just return -EOPNOTSUPP
Changes in v4:
1. Removed PHY_INTERFACE_MODE_2500BASEX, PHY_INTERFACE_MODE_1000BASEX and
2.5G PHY_INTERFACE_MODE_SGMII phy modes from supported modes
Regards,
Parshuram Thombare
Parshuram Thombare (5):
net: macb: add phylink support
net: macb: add support for sgmii MAC-PHY interface
net: macb: add support for c45 PHY
net: macb: add support for high speed interface
net: macb: parameter added to cadence ethernet controller DT binding
.../devicetree/bindings/net/macb.txt | 3 +
drivers/net/ethernet/cadence/Kconfig | 2 +-
drivers/net/ethernet/cadence/macb.h | 113 +++-
drivers/net/ethernet/cadence/macb_main.c | 589 +++++++++++++-----
4 files changed, 524 insertions(+), 183 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH] sis900: increment revision number
From: Daniele Venzano @ 2019-06-23 9:13 UTC (permalink / raw)
To: Joe Perches, Sergej Benilov, netdev
In-Reply-To: <8eb161f4757cc55d7138bf5d30014e8fb8e38a0d.camel@perches.com>
Hello,
I think it is good to know just by looking at the sources that the
driver is still kept up-to-date, so I am in favor of this patch.
Daniele
On 23/06/2019 11:10, Joe Perches wrote:
> On Sun, 2019-06-23 at 09:47 +0200, Sergej Benilov wrote:
>> Increment revision number to 1.08.11 (TX completion fix).
> Better not to bother as the last increment was in 2006.
> The driver version gets the kernel version in any case.
>
>> diff --git a/drivers/net/ethernet/sis/sis900.c b/drivers/net/ethernet/sis/sis900.c
> []
>> @@ -1,6 +1,6 @@
>> /* sis900.c: A SiS 900/7016 PCI Fast Ethernet driver for Linux.
>> Copyright 1999 Silicon Integrated System Corporation
>> - Revision: 1.08.10 Apr. 2 2006
>> + Revision: 1.08.11 Jun. 23 2019
> etc...
>
>
^ permalink raw reply
* Re: [PATCH] sis900: increment revision number
From: Joe Perches @ 2019-06-23 9:10 UTC (permalink / raw)
To: Sergej Benilov, venza, netdev
In-Reply-To: <20190623074707.6348-1-sergej.benilov@googlemail.com>
On Sun, 2019-06-23 at 09:47 +0200, Sergej Benilov wrote:
> Increment revision number to 1.08.11 (TX completion fix).
Better not to bother as the last increment was in 2006.
The driver version gets the kernel version in any case.
> diff --git a/drivers/net/ethernet/sis/sis900.c b/drivers/net/ethernet/sis/sis900.c
[]
> @@ -1,6 +1,6 @@
> /* sis900.c: A SiS 900/7016 PCI Fast Ethernet driver for Linux.
> Copyright 1999 Silicon Integrated System Corporation
> - Revision: 1.08.10 Apr. 2 2006
> + Revision: 1.08.11 Jun. 23 2019
etc...
^ permalink raw reply
* Re: [PATCH] sis900: increment revision number
From: Daniele Venzano @ 2019-06-23 8:57 UTC (permalink / raw)
To: Sergej Benilov, netdev
In-Reply-To: <20190623074707.6348-1-sergej.benilov@googlemail.com>
Sorry to be late in replying, I am ok also with the previous changes.
Signed-off-by: Daniele Venzano <venza@brownhat.org>
On 23/06/2019 09:47, Sergej Benilov wrote:
> Increment revision number to 1.08.11 (TX completion fix).
>
> Signed-off-by: Sergej Benilov <sergej.benilov@googlemail.com>
> ---
> drivers/net/ethernet/sis/sis900.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/sis/sis900.c b/drivers/net/ethernet/sis/sis900.c
> index abb9b42e..09b4e1c5 100644
> --- a/drivers/net/ethernet/sis/sis900.c
> +++ b/drivers/net/ethernet/sis/sis900.c
> @@ -1,6 +1,6 @@
> /* sis900.c: A SiS 900/7016 PCI Fast Ethernet driver for Linux.
> Copyright 1999 Silicon Integrated System Corporation
> - Revision: 1.08.10 Apr. 2 2006
> + Revision: 1.08.11 Jun. 23 2019
>
> Modified from the driver which is originally written by Donald Becker.
>
> @@ -16,7 +16,8 @@
> preliminary Rev. 1.0 Nov. 10, 1998
> SiS 7014 Single Chip 100BASE-TX/10BASE-T Physical Layer Solution,
> preliminary Rev. 1.0 Jan. 18, 1998
> -
> +
> + Rev 1.08.11 Jun. 23 2019 Sergej Benilov TX completion fix
> Rev 1.08.10 Apr. 2 2006 Daniele Venzano add vlan (jumbo packets) support
> Rev 1.08.09 Sep. 19 2005 Daniele Venzano add Wake on LAN support
> Rev 1.08.08 Jan. 22 2005 Daniele Venzano use netif_msg for debugging messages
> @@ -79,7 +80,7 @@
> #include "sis900.h"
>
> #define SIS900_MODULE_NAME "sis900"
> -#define SIS900_DRV_VERSION "v1.08.10 Apr. 2 2006"
> +#define SIS900_DRV_VERSION "v1.08.11 Jun. 23 2019"
>
> static const char version[] =
> KERN_INFO "sis900.c: " SIS900_DRV_VERSION "\n";
^ permalink raw reply
* [PATCH] sis900: increment revision number
From: Sergej Benilov @ 2019-06-23 7:47 UTC (permalink / raw)
To: venza, netdev; +Cc: Sergej Benilov
Increment revision number to 1.08.11 (TX completion fix).
Signed-off-by: Sergej Benilov <sergej.benilov@googlemail.com>
---
drivers/net/ethernet/sis/sis900.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/sis/sis900.c b/drivers/net/ethernet/sis/sis900.c
index abb9b42e..09b4e1c5 100644
--- a/drivers/net/ethernet/sis/sis900.c
+++ b/drivers/net/ethernet/sis/sis900.c
@@ -1,6 +1,6 @@
/* sis900.c: A SiS 900/7016 PCI Fast Ethernet driver for Linux.
Copyright 1999 Silicon Integrated System Corporation
- Revision: 1.08.10 Apr. 2 2006
+ Revision: 1.08.11 Jun. 23 2019
Modified from the driver which is originally written by Donald Becker.
@@ -16,7 +16,8 @@
preliminary Rev. 1.0 Nov. 10, 1998
SiS 7014 Single Chip 100BASE-TX/10BASE-T Physical Layer Solution,
preliminary Rev. 1.0 Jan. 18, 1998
-
+
+ Rev 1.08.11 Jun. 23 2019 Sergej Benilov TX completion fix
Rev 1.08.10 Apr. 2 2006 Daniele Venzano add vlan (jumbo packets) support
Rev 1.08.09 Sep. 19 2005 Daniele Venzano add Wake on LAN support
Rev 1.08.08 Jan. 22 2005 Daniele Venzano use netif_msg for debugging messages
@@ -79,7 +80,7 @@
#include "sis900.h"
#define SIS900_MODULE_NAME "sis900"
-#define SIS900_DRV_VERSION "v1.08.10 Apr. 2 2006"
+#define SIS900_DRV_VERSION "v1.08.11 Jun. 23 2019"
static const char version[] =
KERN_INFO "sis900.c: " SIS900_DRV_VERSION "\n";
--
2.17.1
^ permalink raw reply related
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Ido Schimmel @ 2019-06-23 7:44 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Ido Schimmel, Vivien Didelot, Florian Fainelli,
netdev@vger.kernel.org, Jiri Pirko, andrew@lunn.ch,
davem@davemloft.net
In-Reply-To: <20190623072605.2xqb56tjydqz2jkx@shell.armlinux.org.uk>
On Sun, Jun 23, 2019 at 08:26:05AM +0100, Russell King - ARM Linux admin wrote:
> On Sun, Jun 23, 2019 at 07:09:52AM +0000, Ido Schimmel wrote:
> > When multicast snooping is enabled unregistered multicast traffic should
> > only be flooded to mrouter ports.
>
> Given that IPv6 relies upon multicast working, and multicast snooping
> is a kernel configuration option, and MLD messages will only be sent
> when whenever the configuration on the target changes, and there may
> not be a multicast querier in the system, who does that ensure that
> IPv6 can work on a bridge where the kernel configured and built with
> multicast snooping enabled?
See commit b00589af3b04 ("bridge: disable snooping if there is no
querier"). I think that's unfortunate behavior that we need because
multicast snooping is enabled by default. If it weren't enabled by
default, then anyone enabling it would also make sure there's a querier
in the network.
^ permalink raw reply
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Russell King - ARM Linux admin @ 2019-06-23 7:26 UTC (permalink / raw)
To: Ido Schimmel
Cc: Vivien Didelot, Florian Fainelli, netdev@vger.kernel.org,
Jiri Pirko, andrew@lunn.ch, davem@davemloft.net
In-Reply-To: <20190623070949.GB13466@splinter>
On Sun, Jun 23, 2019 at 07:09:52AM +0000, Ido Schimmel wrote:
> When multicast snooping is enabled unregistered multicast traffic should
> only be flooded to mrouter ports.
Given that IPv6 relies upon multicast working, and multicast snooping
is a kernel configuration option, and MLD messages will only be sent
when whenever the configuration on the target changes, and there may
not be a multicast querier in the system, who does that ensure that
IPv6 can work on a bridge where the kernel configured and built with
multicast snooping enabled?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* [PATCH V1 net-next] net: ena: Fix bug where ring allocation backoff stopped too late
From: sameehj @ 2019-06-23 7:11 UTC (permalink / raw)
To: davem, netdev
Cc: Sameeh Jubran, dwmw, zorik, matua, saeedb, msw, aliguori, nafea,
gtzalik, netanel, alisaidi, benh, akiyano
From: Sameeh Jubran <sameehj@amazon.com>
The current code of create_queues_with_size_backoff() allows the ring size
to become as small as ENA_MIN_RING_SIZE/2. This is a bug since we don't
want the queue ring to be smaller than ENA_MIN_RING_SIZE
In this commit we change the loop's termination condition to look at the
queue size of the next iteration instead of that of the current one,
so that the minimal queue size again becomes ENA_MIN_RING_SIZE.
Fixes: eece4d2ab9d2 ("net: ena: add ethtool function for changing io queue sizes")
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index b7865ee1d..20ec8ff03 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -1839,8 +1839,8 @@ err_setup_tx:
if (cur_rx_ring_size >= cur_tx_ring_size)
new_rx_ring_size = cur_rx_ring_size / 2;
- if (cur_tx_ring_size < ENA_MIN_RING_SIZE ||
- cur_rx_ring_size < ENA_MIN_RING_SIZE) {
+ if (new_tx_ring_size < ENA_MIN_RING_SIZE ||
+ new_rx_ring_size < ENA_MIN_RING_SIZE) {
netif_err(adapter, ifup, adapter->netdev,
"Queue creation failed with the smallest possible queue size of %d for both queues. Not retrying with smaller queues\n",
ENA_MIN_RING_SIZE);
--
2.17.1
^ permalink raw reply related
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Ido Schimmel @ 2019-06-23 7:09 UTC (permalink / raw)
To: Vivien Didelot
Cc: Florian Fainelli, netdev@vger.kernel.org, Jiri Pirko,
linux@armlinux.org.uk, andrew@lunn.ch, davem@davemloft.net
In-Reply-To: <20190621172952.GB9284@t480s.localdomain>
On Fri, Jun 21, 2019 at 05:29:52PM -0400, Vivien Didelot wrote:
> On Thu, 20 Jun 2019 19:24:47 -0700, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > > This patch adds support for enabling or disabling the flooding of
> > > unknown multicast traffic on the CPU ports, depending on the value
> > > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute.
> > >
> > > This allows the user to prevent the CPU to be flooded with a lot of
> > > undesirable traffic that the network stack needs to filter in software.
> > >
> > > The bridge has multicast snooping enabled by default, hence CPU ports
> > > aren't bottlenecked with arbitrary network applications anymore.
> > > But this can be an issue in some scenarios such as pinging the bridge's
> > > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to
> > > 0 would restore unknown multicast flooding and thus fix ICMPv6. As
> > > an alternative, enabling multicast_querier would program the bridge
> > > address into the switch.
> > From what I can read from mlxsw, we should probably also implement the
> > SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent.
> >
> > Since the attribute MC_DISABLED is on the bridge master, we should also
> > iterate over the list of switch ports being a member of that bridge and
> > change their flooding attribute, taking into account whether
> > BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing
> > what mlxsw does here again...
>
> Ouch, doesn't sound like what a driver should be doing :-(
>
> Ido, I cannot find documentation for multicast_snooping or MC_DISABLED
> and the naming isn't clear. Can this be considered as an equivalent
> of mcast_flood but targeting the bridge device itself, describing
> whether the bridge is interested or not in unknown multicast traffic?
Not really, you're only looking at one aspect of this, but there is
more. For example, when multicast snooping is disabled, the MDB is not
considered.
> > Once you act on the user-facing ports, you might be able to leave the
> > CPU port flooding unconditionally, since it would only "flood" the CPU
> > port either because an user-facing port has BR_MCAST_FLOOD set, or
> > because this is known MC traffic that got programmed via the bridge's
> > MDB. Would that work?
>
> You may want the machine or network connected behind a bridge port
> to be flooded with unknown multicast traffic, without having the
> CPU conduit clogged up with this traffic. So these are two distinct
> settings for me.
>
> The only scenario I can think of needing the CPU to be flooded is if
> there's a non-DSA port in the bridge maybe. But IMHO this should be
> handled by the bridge, offloading or not the appropriate attribute.
>
> > On a higher level, I really wish we did not have to re-implement a lot
> > of identical or similar logic in each switch drivers and had a more
> > central model of what is behaviorally expected.
>
> I couldn't agree more, ethernet switch drivers should only offload
> the notified bridge configuration, not noodling their own logic...
Please see my comment to Florian. The driver is doing what needs to be
done in order to comply with the behavior of the bridge driver. As I
wrote to Florian, we can probably move some logic up the stack and
simplify drivers.
> Russell, Ido, Florian, so far I understand that a multicast-unaware
> bridge must flood unknown traffic everywhere (CPU included);
> and a multicast-aware bridge must only flood its ports if their
> mcast_flood is on, and known traffic targeting the bridge must be
> offloaded accordingly. Do you guys agree with this?
When multicast snooping is enabled unregistered multicast traffic should
only be flooded to mrouter ports.
^ permalink raw reply
* [RFC V1 net-next 1/1] net: ena: implement XDP drop support
From: sameehj @ 2019-06-23 7:06 UTC (permalink / raw)
To: davem, netdev
Cc: Sameeh Jubran, dwmw, zorik, matua, saeedb, msw, aliguori, nafea,
gtzalik, netanel, alisaidi, benh, akiyano
In-Reply-To: <20190623070649.18447-1-sameehj@amazon.com>
From: Sameeh Jubran <sameehj@amazon.com>
This commit implements the basic functionality of drop/pass logic in the
ena driver.
Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 83 +++++++++++++++++++-
drivers/net/ethernet/amazon/ena/ena_netdev.h | 29 +++++++
2 files changed, 111 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 20ec8ff03..3d65c0771 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -33,10 +33,10 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#ifdef CONFIG_RFS_ACCEL
+#include <linux/bpf_trace.h>
#include <linux/cpu_rmap.h>
#endif /* CONFIG_RFS_ACCEL */
#include <linux/ethtool.h>
-#include <linux/if_vlan.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/numa.h>
@@ -105,11 +105,82 @@ static void update_rx_ring_mtu(struct ena_adapter *adapter, int mtu)
adapter->rx_ring[i].mtu = mtu;
}
+static int ena_xdp_execute(struct ena_ring *rx_ring, struct xdp_buff *xdp)
+{
+ struct bpf_prog *xdp_prog = rx_ring->xdp_bpf_prog;
+ u32 verdict = XDP_PASS;
+
+ rcu_read_lock();
+
+ if (!xdp_prog)
+ goto out;
+
+ verdict = bpf_prog_run_xdp(xdp_prog, xdp);
+
+ if (unlikely(verdict == XDP_ABORTED))
+ trace_xdp_exception(rx_ring->netdev, xdp_prog, verdict);
+ else if (unlikely(verdict > XDP_REDIRECT))
+ bpf_warn_invalid_xdp_action(verdict);
+out:
+ rcu_read_unlock();
+ return verdict;
+}
+
+static int ena_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
+{
+ struct ena_adapter *adapter = netdev_priv(netdev);
+ struct bpf_prog *old_bpf_prog;
+ int i;
+
+ if (ena_xdp_allowed(adapter)) {
+ old_bpf_prog = xchg(&adapter->xdp_bpf_prog, prog);
+
+ for (i = 0; i < adapter->num_queues; i++)
+ xchg(&adapter->rx_ring[i].xdp_bpf_prog, prog);
+
+ if (old_bpf_prog)
+ bpf_prog_put(old_bpf_prog);
+
+ } else {
+ netif_err(adapter, drv, adapter->netdev, "Failed to set xdp program, the current MTU (%d) is larger than the maximal allowed MTU (%lu) while xdp is on",
+ netdev->mtu, ENA_XDP_MAX_MTU);
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+/* This is the main xdp callback, it's used by the kernel to set/unset the xdp
+ * program as well as to query the current xdp program id.
+ */
+static int ena_xdp(struct net_device *netdev, struct netdev_bpf *bpf)
+{
+ struct ena_adapter *adapter = netdev_priv(netdev);
+
+ switch (bpf->command) {
+ case XDP_SETUP_PROG:
+ return ena_xdp_set(netdev, bpf->prog);
+ case XDP_QUERY_PROG:
+ bpf->prog_id = adapter->xdp_bpf_prog ?
+ adapter->xdp_bpf_prog->aux->id : 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
static int ena_change_mtu(struct net_device *dev, int new_mtu)
{
struct ena_adapter *adapter = netdev_priv(dev);
int ret;
+ if (new_mtu > ENA_XDP_MAX_MTU && ena_xdp_present(adapter)) {
+ netif_err(adapter, drv, dev,
+ "Requested MTU value is not valid while xdp is enabled new_mtu: %d max mtu: %lu min mtu: %d\n",
+ new_mtu, ENA_XDP_MAX_MTU, ENA_MIN_MTU);
+ return -EINVAL;
+ }
ret = ena_com_set_dev_mtu(adapter->ena_dev, new_mtu);
if (!ret) {
netif_dbg(adapter, drv, dev, "set MTU to %d\n", new_mtu);
@@ -888,6 +959,15 @@ static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
va = page_address(rx_info->page) + rx_info->page_offset;
prefetch(va + NET_IP_ALIGN);
+ if (ena_xdp_present_ring(rx_ring)) {
+ rx_ring->xdp.data = va;
+ rx_ring->xdp.data_meta = rx_ring->xdp.data;
+ rx_ring->xdp.data_hard_start = rx_ring->xdp.data -
+ rx_info->page_offset;
+ rx_ring->xdp.data_end = rx_ring->xdp.data + len;
+ if (ena_xdp_execute(rx_ring, &rx_ring->xdp) != XDP_PASS)
+ return NULL;
+ }
if (len <= rx_ring->rx_copybreak) {
skb = ena_alloc_skb(rx_ring, false);
if (unlikely(!skb))
@@ -2549,6 +2629,7 @@ static const struct net_device_ops ena_netdev_ops = {
.ndo_change_mtu = ena_change_mtu,
.ndo_set_mac_address = NULL,
.ndo_validate_addr = eth_validate_addr,
+ .ndo_bpf = ena_xdp,
};
static int ena_device_validate_params(struct ena_adapter *adapter,
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index f2b6e2e05..e17965f7a 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -35,6 +35,7 @@
#include <linux/bitops.h>
#include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
#include <linux/inetdevice.h>
#include <linux/interrupt.h>
#include <linux/netdevice.h>
@@ -139,6 +140,14 @@
#define ENA_MMIO_DISABLE_REG_READ BIT(0)
+/* The max MTU size is configured to be the ethernet frame without the overhead
+ * of the ethernet header, which can have VLAN header, and the frame check
+ * sequence (FCS).
+ * The buffer sizes we share with the device are defined to be ENA_PAGE_SIZE
+ */
+#define ENA_XDP_MAX_MTU (ENA_PAGE_SIZE - ETH_HLEN - ETH_FCS_LEN - \
+ VLAN_HLEN)
+
struct ena_irq {
irq_handler_t handler;
void *data;
@@ -288,6 +297,8 @@ struct ena_ring {
u8 *push_buf_intermediate_buf;
int empty_rx_queue;
+ struct bpf_prog *xdp_bpf_prog;
+ struct xdp_buff xdp;
} ____cacheline_aligned;
struct ena_stats_dev {
@@ -380,6 +391,9 @@ struct ena_adapter {
enum ena_regs_reset_reason_types reset_reason;
u8 ena_extra_properties_count;
+
+ /* XDP structures */
+ struct bpf_prog *xdp_bpf_prog;
};
void ena_set_ethtool_ops(struct net_device *netdev);
@@ -394,4 +408,19 @@ int ena_update_queue_sizes(struct ena_adapter *adapter,
int ena_get_sset_count(struct net_device *netdev, int sset);
+static inline bool ena_xdp_present(struct ena_adapter *adapter)
+{
+ return !!adapter->xdp_bpf_prog;
+}
+
+static inline bool ena_xdp_present_ring(struct ena_ring *ring)
+{
+ return !!ring->xdp_bpf_prog;
+}
+
+static inline bool ena_xdp_allowed(struct ena_adapter *adapter)
+{
+ return adapter->netdev->mtu <= ENA_XDP_MAX_MTU;
+}
+
#endif /* !(ENA_H) */
--
2.17.1
^ permalink raw reply related
* [RFC V1 net-next 0/1] Introduce xdp to ena
From: sameehj @ 2019-06-23 7:06 UTC (permalink / raw)
To: davem, netdev
Cc: Sameeh Jubran, dwmw, zorik, matua, saeedb, msw, aliguori, nafea,
gtzalik, netanel, alisaidi, benh, akiyano
From: Sameeh Jubran <sameehj@amazon.com>
This patch set has one patch which implements xdp drop support in the
ena driver.
Sameeh Jubran (1):
net: ena: implement XDP drop support
drivers/net/ethernet/amazon/ena/ena_netdev.c | 83 +++++++++++++++++++-
drivers/net/ethernet/amazon/ena/ena_netdev.h | 29 +++++++
2 files changed, 111 insertions(+), 1 deletion(-)
--
2.17.1
^ permalink raw reply
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Ido Schimmel @ 2019-06-23 6:48 UTC (permalink / raw)
To: Florian Fainelli
Cc: Vivien Didelot, netdev@vger.kernel.org, Jiri Pirko,
linux@armlinux.org.uk, andrew@lunn.ch, davem@davemloft.net
In-Reply-To: <5d653a4d-3270-8e53-a5e0-88ea5e7a4d3f@gmail.com>
On Thu, Jun 20, 2019 at 07:24:47PM -0700, Florian Fainelli wrote:
> On 6/20/2019 4:56 PM, Vivien Didelot wrote:
> > This patch adds support for enabling or disabling the flooding of
> > unknown multicast traffic on the CPU ports, depending on the value
> > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute.
> >
> > This allows the user to prevent the CPU to be flooded with a lot of
> > undesirable traffic that the network stack needs to filter in software.
> >
> > The bridge has multicast snooping enabled by default, hence CPU ports
> > aren't bottlenecked with arbitrary network applications anymore.
> > But this can be an issue in some scenarios such as pinging the bridge's
> > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to
> > 0 would restore unknown multicast flooding and thus fix ICMPv6. As
> > an alternative, enabling multicast_querier would program the bridge
> > address into the switch.
> From what I can read from mlxsw, we should probably also implement the
> SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent.
>
> Since the attribute MC_DISABLED is on the bridge master, we should also
> iterate over the list of switch ports being a member of that bridge and
> change their flooding attribute, taking into account whether
> BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing
> what mlxsw does here again...
When multicast snooping is enabled, unregistered multicast traffic
should be flooded to mrouter ports only. Otherwise, it should be flooded
to all ports.
> Once you act on the user-facing ports, you might be able to leave the
> CPU port flooding unconditionally, since it would only "flood" the CPU
> port either because an user-facing port has BR_MCAST_FLOOD set, or
> because this is known MC traffic that got programmed via the bridge's
> MDB. Would that work?
>
> On a higher level, I really wish we did not have to re-implement a lot
> of identical or similar logic in each switch drivers and had a more
> central model of what is behaviorally expected.
Well, that model is the bridge driver... But I agree that we can
probably simplify the interface towards drivers and move more code up
the stack.
For example, two things mlxsw is doing when multicast snooping is
enabled:
1. Writing MDB entries to the device. When multicast snooping is
disabled, MDB entries are ignored by the bridge driver. Can we agree to
have the bridge driver generate SWITCHDEV_OBJ_ID_PORT_MDB add / delete
for all MDB entries when multicast snooping is toggled?
2. Flooding unregistered multicast traffic only to mrouter ports. The
bridge driver can iterate over the bridge members and toggle
BR_MCAST_FLOOD accordingly. It will not actually change this value. Only
emulate this change towards drivers.
I will try to come up with a more detailed list later this week. I
really want to start moving more logic out of drivers.
^ permalink raw reply
* RE: [EXT] [PATCH] bnx2x: Prevent ptp_task to be rescheduled indefinitely
From: Sudarsana Reddy Kalluru @ 2019-06-23 5:13 UTC (permalink / raw)
To: Guilherme G. Piccoli, GR-everest-linux-l2, netdev@vger.kernel.org
Cc: Ariel Elior, jay.vosburgh@canonical.com
In-Reply-To: <20190621212634.25441-1-gpiccoli@canonical.com>
> -----Original Message-----
> From: Guilherme G. Piccoli <gpiccoli@canonical.com>
> Sent: Saturday, June 22, 2019 2:57 AM
> To: GR-everest-linux-l2 <GR-everest-linux-l2@marvell.com>;
> netdev@vger.kernel.org
> Cc: Ariel Elior <aelior@marvell.com>; Sudarsana Reddy Kalluru
> <skalluru@marvell.com>; gpiccoli@canonical.com;
> jay.vosburgh@canonical.com
> Subject: [EXT] [PATCH] bnx2x: Prevent ptp_task to be rescheduled indefinitely
>
> External Email
>
> ----------------------------------------------------------------------
> Currently bnx2x ptp worker tries to read a register with timestamp
> information in case of TX packet timestamping and in case it fails, the routine
> reschedules itself indefinitely. This was reported as a kworker always at 100%
> of CPU usage, which was narrowed down to be bnx2x ptp_task.
>
> By following the ioctl handler, we could narrow down the problem to an NTP
> tool (chrony) requesting HW timestamping from bnx2x NIC with RX filter
> zeroed; this isn't reproducible for example with linuxptp since this tool
> request a supported RX filter. It seems the NIC HW timestamp mechanism
> cannot work well with RX_FILTER_NONE - in driver's PTP filter initialization
> routine, when there's not a supported filter request the function does not
> perform a specific register write to the adapter.
>
> This patch addresses the problem of the everlasting reschedule of the ptp
> worker by limiting that to 3 attempts (the first one plus two reschedules), in
> order to prevent the unbound resource consumption from the driver. It's not
> correct behavior for a driver to not take into account potential problems in a
> routine reading a device register, be it an invalid RX filter (leading to a non-
> functional HW clock) or even a potential device FW issue causing the register
> value to be wrong, hence we believe the fix is relevant to ensure proper
> driver behavior.
>
> This has no functional change in the succeeding path of the HW
> timestamping code in the driver, only portion of code it changes is the error
> path for TX timestamping. It was tested using both linuxptp and chrony.
>
> Reported-and-tested-by: Przemyslaw Hausman
> <przemyslaw.hausman@canonical.com>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
> ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 1 +
> .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 1 +
> .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 18 +++++++++++++-----
> 3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> index 6026b53137aa..349965135227 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> @@ -1838,6 +1838,7 @@ struct bnx2x {
> bool timecounter_init_done;
> struct sk_buff *ptp_tx_skb;
> unsigned long ptp_tx_start;
> + u8 ptp_retry_count;
> bool hwtstamp_ioctl_called;
> u16 tx_type;
> u16 rx_filter;
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> index 008ad0ca89ba..990ec049f357 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -3865,6 +3865,7 @@ netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> /* schedule check for Tx timestamp */
> bp->ptp_tx_skb = skb_get(skb);
> bp->ptp_tx_start = jiffies;
> + bp->ptp_retry_count = 0;
> schedule_work(&bp->ptp_task);
> }
> }
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index 03ac10b1cd1e..872ae672faaa 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -15233,16 +15233,24 @@ static void bnx2x_ptp_task(struct work_struct
> *work)
> memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> shhwtstamps.hwtstamp = ns_to_ktime(ns);
> skb_tstamp_tx(bp->ptp_tx_skb, &shhwtstamps);
> - dev_kfree_skb_any(bp->ptp_tx_skb);
> - bp->ptp_tx_skb = NULL;
> -
> DP(BNX2X_MSG_PTP, "Tx timestamp, timestamp cycles =
> %llu, ns = %llu\n",
> timestamp, ns);
> + goto clear;
> } else {
> DP(BNX2X_MSG_PTP, "There is no valid Tx timestamp
> yet\n");
> - /* Reschedule to keep checking for a valid timestamp value
> */
> - schedule_work(&bp->ptp_task);
> + /* Reschedule twice to check again for a valid timestamp */
> + if (++bp->ptp_retry_count < 3) {
> + schedule_work(&bp->ptp_task);
> + return;
> + }
> + DP(BNX2X_MSG_PTP, "Gave up Tx timestamp, register read
> %u\n", val_seq);
> + netdev_warn_once(bp->dev,
> + "Gave up Tx timestamp, register read %u\n",
> val_seq);
> }
> +clear:
> + dev_kfree_skb_any(bp->ptp_tx_skb);
> + bp->ptp_tx_skb = NULL;
> + bp->ptp_retry_count = 0;
> }
>
> void bnx2x_set_rx_ts(struct bnx2x *bp, struct sk_buff *skb)
> --
> 2.21.0
Thanks for uncovering this issue, and the fix.
With the proposed fix, if HW latches the timestamp after 3 iterations then it would lead to erroneous PTP functionality. When driver receives the next PTP packet, driver sees that timestamp is available in the register and would assign this value (which corresponds to last/skipped ptp packet) to the PTP packet. In general, FW takes around 1ms to 100ms to perform the timestamp and may take more. Hence the promising solution for this issue would be to wait for timestamp for particular period of time. If Tx timestamp is not available for a pre-determined max period, then declare it as an error scenario and terminate the thread.
Just for the reference, similar fix was added for Marvell qede driver recently. Patch details are,
9adebac37e7d: (qede: Handle infinite driver spinning for Tx timestamp.)
https://www.spinics.net/lists/netdev/msg572838.html
^ permalink raw reply
* Re: [PATCH net-next v5 5/7] taprio: Add support for txtime-assist mode
From: kbuild test robot @ 2019-06-23 3:29 UTC (permalink / raw)
To: Vedang Patel
Cc: kbuild-all, netdev, jeffrey.t.kirsher, davem, jhs, xiyou.wangcong,
jiri, intel-wired-lan, vinicius.gomes, l, jakub.kicinski,
m-karicheri2, sergei.shtylyov, eric.dumazet, aaron.f.brown,
Vedang Patel
In-Reply-To: <1561138108-12943-6-git-send-email-vedang.patel@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]
Hi Vedang,
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/Vedang-Patel/net-sched-Add-txtime-assist-support-for-taprio/20190622-192248
config: i386-randconfig-h1-06230338 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ld: net/sched/sch_taprio.o: in function `length_to_duration':
>> net/sched/sch_taprio.c:154: undefined reference to `__divdi3'
>> ld: net/sched/sch_taprio.c:154: undefined reference to `__divdi3'
>> ld: net/sched/sch_taprio.c:154: undefined reference to `__divdi3'
vim +154 net/sched/sch_taprio.c
151
152 static int length_to_duration(struct taprio_sched *q, int len)
153 {
> 154 return (len * atomic64_read(&q->picos_per_byte)) / 1000;
155 }
156
---
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: 29195 bytes --]
^ permalink raw reply
* [PATCH v3] e1000e: Make watchdog use delayed work
From: Detlev Casanova @ 2019-06-23 3:14 UTC (permalink / raw)
To: Jeff Kirsher, David S. Miller, intel-wired-lan, netdev,
linux-kernel
Cc: Detlev Casanova
Use delayed work instead of timers to run the watchdog of the e1000e
driver.
Simplify the code with one less middle function.
Signed-off-by: Detlev Casanova <detlev.casanova@gmail.com>
---
drivers/net/ethernet/intel/e1000e/e1000.h | 5 +-
drivers/net/ethernet/intel/e1000e/netdev.c | 54 ++++++++++++----------
2 files changed, 32 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index be13227f1697..34cd67951aec 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -186,12 +186,13 @@ struct e1000_phy_regs {
/* board specific private data structure */
struct e1000_adapter {
- struct timer_list watchdog_timer;
struct timer_list phy_info_timer;
struct timer_list blink_timer;
struct work_struct reset_task;
- struct work_struct watchdog_task;
+ struct delayed_work watchdog_task;
+
+ struct workqueue_struct *e1000_workqueue;
const struct e1000_info *ei;
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 0e09bede42a2..6fb6918bc136 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -1780,7 +1780,8 @@ static irqreturn_t e1000_intr_msi(int __always_unused irq, void *data)
}
/* guard against interrupt when we're going down */
if (!test_bit(__E1000_DOWN, &adapter->state))
- mod_timer(&adapter->watchdog_timer, jiffies + 1);
+ queue_delayed_work(adapter->e1000_workqueue,
+ &adapter->watchdog_task, 1);
}
/* Reset on uncorrectable ECC error */
@@ -1860,7 +1861,8 @@ static irqreturn_t e1000_intr(int __always_unused irq, void *data)
}
/* guard against interrupt when we're going down */
if (!test_bit(__E1000_DOWN, &adapter->state))
- mod_timer(&adapter->watchdog_timer, jiffies + 1);
+ queue_delayed_work(adapter->e1000_workqueue,
+ &adapter->watchdog_task, 1);
}
/* Reset on uncorrectable ECC error */
@@ -1905,7 +1907,8 @@ static irqreturn_t e1000_msix_other(int __always_unused irq, void *data)
hw->mac.get_link_status = true;
/* guard against interrupt when we're going down */
if (!test_bit(__E1000_DOWN, &adapter->state))
- mod_timer(&adapter->watchdog_timer, jiffies + 1);
+ queue_delayed_work(adapter->e1000_workqueue,
+ &adapter->watchdog_task, 1);
}
if (!test_bit(__E1000_DOWN, &adapter->state))
@@ -4278,7 +4281,6 @@ void e1000e_down(struct e1000_adapter *adapter, bool reset)
napi_synchronize(&adapter->napi);
- del_timer_sync(&adapter->watchdog_timer);
del_timer_sync(&adapter->phy_info_timer);
spin_lock(&adapter->stats64_lock);
@@ -5150,25 +5152,11 @@ static void e1000e_check_82574_phy_workaround(struct e1000_adapter *adapter)
}
}
-/**
- * e1000_watchdog - Timer Call-back
- * @data: pointer to adapter cast into an unsigned long
- **/
-static void e1000_watchdog(struct timer_list *t)
-{
- struct e1000_adapter *adapter = from_timer(adapter, t, watchdog_timer);
-
- /* Do the rest outside of interrupt context */
- schedule_work(&adapter->watchdog_task);
-
- /* TODO: make this use queue_delayed_work() */
-}
-
static void e1000_watchdog_task(struct work_struct *work)
{
struct e1000_adapter *adapter = container_of(work,
struct e1000_adapter,
- watchdog_task);
+ watchdog_task.work);
struct net_device *netdev = adapter->netdev;
struct e1000_mac_info *mac = &adapter->hw.mac;
struct e1000_phy_info *phy = &adapter->hw.phy;
@@ -5395,8 +5383,9 @@ static void e1000_watchdog_task(struct work_struct *work)
/* Reset the timer */
if (!test_bit(__E1000_DOWN, &adapter->state))
- mod_timer(&adapter->watchdog_timer,
- round_jiffies(jiffies + 2 * HZ));
+ queue_delayed_work(adapter->e1000_workqueue,
+ &adapter->watchdog_task,
+ round_jiffies(2 * HZ));
}
#define E1000_TX_FLAGS_CSUM 0x00000001
@@ -7251,11 +7240,21 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto err_eeprom;
}
- timer_setup(&adapter->watchdog_timer, e1000_watchdog, 0);
+ adapter->e1000_workqueue = alloc_workqueue("%s", WQ_MEM_RECLAIM, 0,
+ e1000e_driver_name);
+
+ if (!adapter->e1000_workqueue) {
+ err = -ENOMEM;
+ goto err_workqueue;
+ }
+
+ INIT_DELAYED_WORK(&adapter->watchdog_task, e1000_watchdog_task);
+ queue_delayed_work(adapter->e1000_workqueue, &adapter->watchdog_task,
+ 0);
+
timer_setup(&adapter->phy_info_timer, e1000_update_phy_info, 0);
INIT_WORK(&adapter->reset_task, e1000_reset_task);
- INIT_WORK(&adapter->watchdog_task, e1000_watchdog_task);
INIT_WORK(&adapter->downshift_task, e1000e_downshift_workaround);
INIT_WORK(&adapter->update_phy_task, e1000e_update_phy_task);
INIT_WORK(&adapter->print_hang_task, e1000_print_hw_hang);
@@ -7349,6 +7348,9 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
return 0;
err_register:
+ flush_workqueue(adapter->e1000_workqueue);
+ destroy_workqueue(adapter->e1000_workqueue);
+err_workqueue:
if (!(adapter->flags & FLAG_HAS_AMT))
e1000e_release_hw_control(adapter);
err_eeprom:
@@ -7395,15 +7397,17 @@ static void e1000_remove(struct pci_dev *pdev)
*/
if (!down)
set_bit(__E1000_DOWN, &adapter->state);
- del_timer_sync(&adapter->watchdog_timer);
del_timer_sync(&adapter->phy_info_timer);
cancel_work_sync(&adapter->reset_task);
- cancel_work_sync(&adapter->watchdog_task);
cancel_work_sync(&adapter->downshift_task);
cancel_work_sync(&adapter->update_phy_task);
cancel_work_sync(&adapter->print_hang_task);
+ cancel_delayed_work(&adapter->watchdog_task);
+ flush_workqueue(adapter->e1000_workqueue);
+ destroy_workqueue(adapter->e1000_workqueue);
+
if (adapter->flags & FLAG_HAS_HW_TIMESTAMP) {
cancel_work_sync(&adapter->tx_hwtstamp_work);
if (adapter->tx_hwtstamp_skb) {
--
2.22.0
^ permalink raw reply related
* Re: [RFC PATCH 0/2] enable broadcom tagging for bcm531x5 switches
From: Florian Fainelli @ 2019-06-23 2:24 UTC (permalink / raw)
To: Benedikt Spranger; +Cc: netdev, Sebastian Andrzej Siewior, Kurt Kanzenbach
In-Reply-To: <20190619111832.16935a93@mitra>
[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]
On 6/19/2019 2:18 AM, Benedikt Spranger wrote:
> On Tue, 18 Jun 2019 11:16:23 -0700
> Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>> How is that a problem for other machines? Does that lead to some kind
>> of broadcast storm because there are machines that keep trying to
>> respond to ARP solicitations?
> Mirroring broadcast packages on the interface they are coming in, is
> IMHO a poor idea. As a result any switch connected to wan update the
> MAC table and send packages on a port where they do not belong to.
> Just imagine to send a DHCP request. The BPi R1 acts as nearly perfect
> black hole in such a situation.
Fair enough.
>
>> The few aspects that bother me, not in any particular order, are that:
>>
>> - you might be able to not change anything and just get away with the
>> one line patch below that sets skb->offload_fwd_mark to 1 to indicate
>> to the bridge, not to bother with sending a copy of the packet, since
>> the HW took care of that already
>
> I can test it, but i like to note that the changed function is not
> executed in case of bcm53125.
Indeed, that won't work, how about implementing port_egress_flood() for
b53? That would make sure that multicast is flooded (as well as unicast)
before the switch port is enslaved into the bridge and this should take
care of both your problem and the lack of multicast flooding once
Broadcom tags are turned on. You might also need to set
skb->fwd_offload_mark accordingly in case you still see duplicate
packets, though that should not happen anymore AFAICT.
Something like this should take care of that (untested). You might have
to explicitly set the IMP port (port 8) in B53_UC_FWD_EN and
B53_MC_FWD_EN, though since you turn on management mode, this may not be
required. I might have some time tomorrow to test that on a Lamobo R1.
--
Florian
[-- Attachment #2: b53-egress-floods.patch --]
[-- Type: text/plain, Size: 2684 bytes --]
diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index c8040ecf4425..a47f5bc667bd 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -342,6 +342,13 @@ static void b53_set_forwarding(struct b53_device *dev, int enable)
b53_read8(dev, B53_CTRL_PAGE, B53_SWITCH_CTRL, &mgmt);
mgmt |= B53_MII_DUMB_FWDG_EN;
b53_write8(dev, B53_CTRL_PAGE, B53_SWITCH_CTRL, mgmt);
+
+ /* Look at B53_UC_FWD_EN and B53_MC_FWD_EN to decide whether
+ * frames should be flooed or not.
+ */
+ b53_read8(dev, B53_CTRL_PAGE, B53_IP_MULTICAST_CTRL, &mgmt);
+ mgmt |= B53_UC_FWD_EN | B53_MC_FWD_EN;
+ b53_write8(dev, B53_CTRL_PAGE, B53_IP_MULTICAST_CTRL, mgmt);
}
static void b53_enable_vlan(struct b53_device *dev, bool enable,
@@ -1748,6 +1755,31 @@ void b53_br_fast_age(struct dsa_switch *ds, int port)
}
EXPORT_SYMBOL(b53_br_fast_age);
+int b53_br_egress_floods(struct dsa_switch *ds, int port,
+ bool unicast, bool multicast)
+{
+ struct b53_device *dev = ds->priv;
+ u16 uc, mc;
+
+ b53_read16(dev, B53_CTRL_PAGE, B53_UC_FWD_EN, &uc);
+ if (unicast)
+ uc |= BIT(port);
+ else
+ uc &= ~BIT(port);
+ b53_write16(dev, B53_CTRL_PAGE, B53_UC_FWD_EN, uc);
+
+ b53_read16(dev, B53_CTRL_PAGE, B53_MC_FWD_EN, &mc);
+ if (multicast)
+ mc |= BIT(port);
+ else
+ mc &= ~BIT(port);
+ b53_write16(dev, B53_CTRL_PAGE, B53_MC_FWD_EN, mc);
+
+ return 0;
+
+}
+EXPORT_SYMBOL(b53_br_egress_floods);
+
static bool b53_possible_cpu_port(struct dsa_switch *ds, int port)
{
/* Broadcom switches will accept enabling Broadcom tags on the
@@ -1948,6 +1980,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
.port_bridge_leave = b53_br_leave,
.port_stp_state_set = b53_br_set_stp_state,
.port_fast_age = b53_br_fast_age,
+ .port_egress_floods = b53_br_egress_floods,
.port_vlan_filtering = b53_vlan_filtering,
.port_vlan_prepare = b53_vlan_prepare,
.port_vlan_add = b53_vlan_add,
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index f25bc80c4ffc..a7dd8acc281b 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -319,6 +319,8 @@ int b53_br_join(struct dsa_switch *ds, int port, struct net_device *bridge);
void b53_br_leave(struct dsa_switch *ds, int port, struct net_device *bridge);
void b53_br_set_stp_state(struct dsa_switch *ds, int port, u8 state);
void b53_br_fast_age(struct dsa_switch *ds, int port);
+int b53_br_egress_floods(struct dsa_switch *ds, int port,
+ bool unicast, bool multicast);
void b53_port_event(struct dsa_switch *ds, int port);
void b53_phylink_validate(struct dsa_switch *ds, int port,
unsigned long *supported,
^ permalink raw reply related
* Re: [PATCH v2 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Willem de Bruijn @ 2019-06-23 2:21 UTC (permalink / raw)
To: Neil Horman; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <CAF=yD-JC_r1vjitN1ccyvQ3DXiP9BNCwq9iiWU11cXznmhAY8Q@mail.gmail.com>
> > -static void __packet_set_status(struct packet_sock *po, void *frame, int status)
> > +static void __packet_set_status(struct packet_sock *po, void *frame, int status,
> > + bool call_complete)
> > {
> > union tpacket_uhdr h;
> >
> > @@ -381,6 +382,8 @@ static void __packet_set_status(struct packet_sock *po, void *frame, int status)
> > BUG();
> > }
> >
> > + if (po->wait_on_complete && call_complete)
> > + complete(&po->skb_completion);
>
> This wake need not happen before the barrier. Only one caller of
> __packet_set_status passes call_complete (tpacket_destruct_skb).
> Moving this branch to the caller avoids a lot of code churn.
>
> Also, multiple packets may be released before the process is awoken.
> The process will block until packet_read_pending drops to zero. Can
> defer the wait_on_complete to that one instance.
Eh no. The point of having this sleep in the send loop is that
additional slots may be released for transmission (flipped to
TP_STATUS_SEND_REQUEST) from another thread while this thread is
waiting.
Else, it would have been much simpler to move the wait below the send
loop: send as many packets as possible, then wait for all of them
having been released. Much clearer control flow.
Where to set and clear the wait_on_complete boolean remains. Integer
assignment is fragile, as the compiler and processor may optimize or
move simple seemingly independent operations. As complete() takes a
spinlock, avoiding that in the DONTWAIT case is worthwhile. But probably
still preferable to set when beginning waiting and clear when calling
complete.
^ permalink raw reply
* [PATCH bpf-next v5 2/3] bpf_xdp_redirect_map: Perform map lookup in eBPF helper
From: Toke Høiland-Jørgensen @ 2019-06-23 2:17 UTC (permalink / raw)
To: netdev
Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
David Miller, Jonathan Lemon
In-Reply-To: <156125626076.5209.13424524054109901554.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
The bpf_redirect_map() helper used by XDP programs doesn't return any
indication of whether it can successfully redirect to the map index it was
given. Instead, BPF programs have to track this themselves, leading to
programs using duplicate maps to track which entries are populated in the
devmap.
This patch fixes this by moving the map lookup into the bpf_redirect_map()
helper, which makes it possible to return failure to the eBPF program. The
lower bits of the flags argument is used as the return code, which means
that existing users who pass a '0' flag argument will get XDP_ABORTED.
With this, a BPF program can check the return code from the helper call and
react by, for instance, substituting a different redirect. This works for
any type of map used for redirect.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
include/linux/filter.h | 1 +
include/uapi/linux/bpf.h | 7 +++++--
net/core/filter.c | 27 +++++++++++++--------------
3 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 43b45d6db36d..f31ae8b9035a 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -580,6 +580,7 @@ struct bpf_skb_data_end {
struct bpf_redirect_info {
u32 ifindex;
u32 flags;
+ void *item;
struct bpf_map *map;
struct bpf_map *map_to_flush;
u32 kern_flags;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b077507efa3f..59f3fe61b2b0 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1568,8 +1568,11 @@ union bpf_attr {
* but this is only implemented for native XDP (with driver
* support) as of this writing).
*
- * All values for *flags* are reserved for future usage, and must
- * be left at zero.
+ * The lower two bits of *flags* are used as the return code if
+ * the map lookup fails. This is so that the return value can be
+ * one of the XDP program return codes up to XDP_TX, as chosen by
+ * the caller. Any higher bits in the *flags* argument must be
+ * unset.
*
* When used to redirect packets to net devices, this helper
* provides a high performance increase over **bpf_redirect**\ ().
diff --git a/net/core/filter.c b/net/core/filter.c
index 183bf4d8e301..a6779e1cc1b8 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3605,17 +3605,13 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
struct bpf_redirect_info *ri)
{
u32 index = ri->ifindex;
- void *fwd = NULL;
+ void *fwd = ri->item;
int err;
ri->ifindex = 0;
+ ri->item = NULL;
WRITE_ONCE(ri->map, NULL);
- fwd = __xdp_map_lookup_elem(map, index);
- if (unlikely(!fwd)) {
- err = -EINVAL;
- goto err;
- }
if (ri->map_to_flush && unlikely(ri->map_to_flush != map))
xdp_do_flush_map();
@@ -3652,18 +3648,13 @@ static int xdp_do_generic_redirect_map(struct net_device *dev,
{
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
u32 index = ri->ifindex;
- void *fwd = NULL;
+ void *fwd = ri->item;
int err = 0;
ri->ifindex = 0;
+ ri->item = NULL;
WRITE_ONCE(ri->map, NULL);
- fwd = __xdp_map_lookup_elem(map, index);
- if (unlikely(!fwd)) {
- err = -EINVAL;
- goto err;
- }
-
if (map->map_type == BPF_MAP_TYPE_DEVMAP) {
struct bpf_dtab_netdev *dst = fwd;
@@ -3732,6 +3723,7 @@ BPF_CALL_2(bpf_xdp_redirect, u32, ifindex, u64, flags)
ri->ifindex = ifindex;
ri->flags = flags;
+ ri->item = NULL;
WRITE_ONCE(ri->map, NULL);
return XDP_REDIRECT;
@@ -3750,9 +3742,16 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
{
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
- if (unlikely(flags))
+ /* Lower bits of the flags are used as return code on lookup failure */
+ if (unlikely(flags > XDP_TX))
return XDP_ABORTED;
+ ri->item = __xdp_map_lookup_elem(map, ifindex);
+ if (unlikely(!ri->item)) {
+ WRITE_ONCE(ri->map, NULL);
+ return flags;
+ }
+
ri->ifindex = ifindex;
ri->flags = flags;
WRITE_ONCE(ri->map, map);
^ permalink raw reply related
* [PATCH bpf-next v5 1/3] devmap/cpumap: Use flush list instead of bitmap
From: Toke Høiland-Jørgensen @ 2019-06-23 2:17 UTC (permalink / raw)
To: netdev
Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
David Miller, Jonathan Lemon
In-Reply-To: <156125626076.5209.13424524054109901554.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
The socket map uses a linked list instead of a bitmap to keep track of
which entries to flush. Do the same for devmap and cpumap, as this means we
don't have to care about the map index when enqueueing things into the
map (and so we can cache the map lookup).
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
kernel/bpf/cpumap.c | 106 +++++++++++++++++++++++---------------------------
kernel/bpf/devmap.c | 108 +++++++++++++++++++++++----------------------------
net/core/filter.c | 2 -
3 files changed, 97 insertions(+), 119 deletions(-)
diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index 8dff08768087..e53daa552abc 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -32,14 +32,19 @@
/* General idea: XDP packets getting XDP redirected to another CPU,
* will maximum be stored/queued for one driver ->poll() call. It is
- * guaranteed that setting flush bit and flush operation happen on
+ * guaranteed that queueing the frame and the flush operation happen on
* same CPU. Thus, cpu_map_flush operation can deduct via this_cpu_ptr()
* which queue in bpf_cpu_map_entry contains packets.
*/
#define CPU_MAP_BULK_SIZE 8 /* 8 == one cacheline on 64-bit archs */
+struct bpf_cpu_map_entry;
+struct bpf_cpu_map;
+
struct xdp_bulk_queue {
void *q[CPU_MAP_BULK_SIZE];
+ struct list_head flush_node;
+ struct bpf_cpu_map_entry *obj;
unsigned int count;
};
@@ -52,6 +57,8 @@ struct bpf_cpu_map_entry {
/* XDP can run multiple RX-ring queues, need __percpu enqueue store */
struct xdp_bulk_queue __percpu *bulkq;
+ struct bpf_cpu_map *cmap;
+
/* Queue with potential multi-producers, and single-consumer kthread */
struct ptr_ring *queue;
struct task_struct *kthread;
@@ -65,23 +72,17 @@ struct bpf_cpu_map {
struct bpf_map map;
/* Below members specific for map type */
struct bpf_cpu_map_entry **cpu_map;
- unsigned long __percpu *flush_needed;
+ struct list_head __percpu *flush_list;
};
-static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
- struct xdp_bulk_queue *bq, bool in_napi_ctx);
-
-static u64 cpu_map_bitmap_size(const union bpf_attr *attr)
-{
- return BITS_TO_LONGS(attr->max_entries) * sizeof(unsigned long);
-}
+static int bq_flush_to_queue(struct xdp_bulk_queue *bq, bool in_napi_ctx);
static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
{
struct bpf_cpu_map *cmap;
int err = -ENOMEM;
+ int ret, cpu;
u64 cost;
- int ret;
if (!capable(CAP_SYS_ADMIN))
return ERR_PTR(-EPERM);
@@ -105,7 +106,7 @@ static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
/* make sure page count doesn't overflow */
cost = (u64) cmap->map.max_entries * sizeof(struct bpf_cpu_map_entry *);
- cost += cpu_map_bitmap_size(attr) * num_possible_cpus();
+ cost += sizeof(struct list_head) * num_possible_cpus();
/* Notice returns -EPERM on if map size is larger than memlock limit */
ret = bpf_map_charge_init(&cmap->map.memory, cost);
@@ -114,12 +115,13 @@ static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
goto free_cmap;
}
- /* A per cpu bitfield with a bit per possible CPU in map */
- cmap->flush_needed = __alloc_percpu(cpu_map_bitmap_size(attr),
- __alignof__(unsigned long));
- if (!cmap->flush_needed)
+ cmap->flush_list = alloc_percpu(struct list_head);
+ if (!cmap->flush_list)
goto free_charge;
+ for_each_possible_cpu(cpu)
+ INIT_LIST_HEAD(per_cpu_ptr(cmap->flush_list, cpu));
+
/* Alloc array for possible remote "destination" CPUs */
cmap->cpu_map = bpf_map_area_alloc(cmap->map.max_entries *
sizeof(struct bpf_cpu_map_entry *),
@@ -129,7 +131,7 @@ static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
return &cmap->map;
free_percpu:
- free_percpu(cmap->flush_needed);
+ free_percpu(cmap->flush_list);
free_charge:
bpf_map_charge_finish(&cmap->map.memory);
free_cmap:
@@ -334,7 +336,8 @@ static struct bpf_cpu_map_entry *__cpu_map_entry_alloc(u32 qsize, u32 cpu,
{
gfp_t gfp = GFP_KERNEL | __GFP_NOWARN;
struct bpf_cpu_map_entry *rcpu;
- int numa, err;
+ struct xdp_bulk_queue *bq;
+ int numa, err, i;
/* Have map->numa_node, but choose node of redirect target CPU */
numa = cpu_to_node(cpu);
@@ -349,6 +352,11 @@ static struct bpf_cpu_map_entry *__cpu_map_entry_alloc(u32 qsize, u32 cpu,
if (!rcpu->bulkq)
goto free_rcu;
+ for_each_possible_cpu(i) {
+ bq = per_cpu_ptr(rcpu->bulkq, i);
+ bq->obj = rcpu;
+ }
+
/* Alloc queue */
rcpu->queue = kzalloc_node(sizeof(*rcpu->queue), gfp, numa);
if (!rcpu->queue)
@@ -405,7 +413,7 @@ static void __cpu_map_entry_free(struct rcu_head *rcu)
struct xdp_bulk_queue *bq = per_cpu_ptr(rcpu->bulkq, cpu);
/* No concurrent bq_enqueue can run at this point */
- bq_flush_to_queue(rcpu, bq, false);
+ bq_flush_to_queue(bq, false);
}
free_percpu(rcpu->bulkq);
/* Cannot kthread_stop() here, last put free rcpu resources */
@@ -488,6 +496,7 @@ static int cpu_map_update_elem(struct bpf_map *map, void *key, void *value,
rcpu = __cpu_map_entry_alloc(qsize, key_cpu, map->id);
if (!rcpu)
return -ENOMEM;
+ rcpu->cmap = cmap;
}
rcu_read_lock();
__cpu_map_entry_replace(cmap, key_cpu, rcpu);
@@ -514,14 +523,14 @@ static void cpu_map_free(struct bpf_map *map)
synchronize_rcu();
/* To ensure all pending flush operations have completed wait for flush
- * bitmap to indicate all flush_needed bits to be zero on _all_ cpus.
- * Because the above synchronize_rcu() ensures the map is disconnected
- * from the program we can assume no new bits will be set.
+ * list be empty on _all_ cpus. Because the above synchronize_rcu()
+ * ensures the map is disconnected from the program we can assume no new
+ * items will be added to the list.
*/
for_each_online_cpu(cpu) {
- unsigned long *bitmap = per_cpu_ptr(cmap->flush_needed, cpu);
+ struct list_head *flush_list = per_cpu_ptr(cmap->flush_list, cpu);
- while (!bitmap_empty(bitmap, cmap->map.max_entries))
+ while (!list_empty(flush_list))
cond_resched();
}
@@ -538,7 +547,7 @@ static void cpu_map_free(struct bpf_map *map)
/* bq flush and cleanup happens after RCU graze-period */
__cpu_map_entry_replace(cmap, i, NULL); /* call_rcu */
}
- free_percpu(cmap->flush_needed);
+ free_percpu(cmap->flush_list);
bpf_map_area_free(cmap->cpu_map);
kfree(cmap);
}
@@ -590,9 +599,9 @@ const struct bpf_map_ops cpu_map_ops = {
.map_check_btf = map_check_no_btf,
};
-static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
- struct xdp_bulk_queue *bq, bool in_napi_ctx)
+static int bq_flush_to_queue(struct xdp_bulk_queue *bq, bool in_napi_ctx)
{
+ struct bpf_cpu_map_entry *rcpu = bq->obj;
unsigned int processed = 0, drops = 0;
const int to_cpu = rcpu->cpu;
struct ptr_ring *q;
@@ -621,6 +630,9 @@ static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
bq->count = 0;
spin_unlock(&q->producer_lock);
+ __list_del(bq->flush_node.prev, bq->flush_node.next);
+ bq->flush_node.prev = NULL;
+
/* Feedback loop via tracepoints */
trace_xdp_cpumap_enqueue(rcpu->map_id, processed, drops, to_cpu);
return 0;
@@ -631,10 +643,11 @@ static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
*/
static int bq_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_frame *xdpf)
{
+ struct list_head *flush_list = this_cpu_ptr(rcpu->cmap->flush_list);
struct xdp_bulk_queue *bq = this_cpu_ptr(rcpu->bulkq);
if (unlikely(bq->count == CPU_MAP_BULK_SIZE))
- bq_flush_to_queue(rcpu, bq, true);
+ bq_flush_to_queue(bq, true);
/* Notice, xdp_buff/page MUST be queued here, long enough for
* driver to code invoking us to finished, due to driver
@@ -646,6 +659,10 @@ static int bq_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_frame *xdpf)
* operation, when completing napi->poll call.
*/
bq->q[bq->count++] = xdpf;
+
+ if (!bq->flush_node.prev)
+ list_add(&bq->flush_node, flush_list);
+
return 0;
}
@@ -665,41 +682,16 @@ int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp,
return 0;
}
-void __cpu_map_insert_ctx(struct bpf_map *map, u32 bit)
-{
- struct bpf_cpu_map *cmap = container_of(map, struct bpf_cpu_map, map);
- unsigned long *bitmap = this_cpu_ptr(cmap->flush_needed);
-
- __set_bit(bit, bitmap);
-}
-
void __cpu_map_flush(struct bpf_map *map)
{
struct bpf_cpu_map *cmap = container_of(map, struct bpf_cpu_map, map);
- unsigned long *bitmap = this_cpu_ptr(cmap->flush_needed);
- u32 bit;
-
- /* The napi->poll softirq makes sure __cpu_map_insert_ctx()
- * and __cpu_map_flush() happen on same CPU. Thus, the percpu
- * bitmap indicate which percpu bulkq have packets.
- */
- for_each_set_bit(bit, bitmap, map->max_entries) {
- struct bpf_cpu_map_entry *rcpu = READ_ONCE(cmap->cpu_map[bit]);
- struct xdp_bulk_queue *bq;
-
- /* This is possible if entry is removed by user space
- * between xdp redirect and flush op.
- */
- if (unlikely(!rcpu))
- continue;
-
- __clear_bit(bit, bitmap);
+ struct list_head *flush_list = this_cpu_ptr(cmap->flush_list);
+ struct xdp_bulk_queue *bq, *tmp;
- /* Flush all frames in bulkq to real queue */
- bq = this_cpu_ptr(rcpu->bulkq);
- bq_flush_to_queue(rcpu, bq, true);
+ list_for_each_entry_safe(bq, tmp, flush_list, flush_node) {
+ bq_flush_to_queue(bq, true);
/* If already running, costs spin_lock_irqsave + smb_mb */
- wake_up_process(rcpu->kthread);
+ wake_up_process(bq->obj->kthread);
}
}
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 40e86a7e0ef0..59113bb108c7 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -17,9 +17,8 @@
* datapath always has a valid copy. However, the datapath does a "flush"
* operation that pushes any pending packets in the driver outside the RCU
* critical section. Each bpf_dtab_netdev tracks these pending operations using
- * an atomic per-cpu bitmap. The bpf_dtab_netdev object will not be destroyed
- * until all bits are cleared indicating outstanding flush operations have
- * completed.
+ * a per-cpu flush list. The bpf_dtab_netdev object will not be destroyed until
+ * this list is empty, indicating outstanding flush operations have completed.
*
* BPF syscalls may race with BPF program calls on any of the update, delete
* or lookup operations. As noted above the xchg() operation also keep the
@@ -48,9 +47,13 @@
(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
#define DEV_MAP_BULK_SIZE 16
+struct bpf_dtab_netdev;
+
struct xdp_bulk_queue {
struct xdp_frame *q[DEV_MAP_BULK_SIZE];
+ struct list_head flush_node;
struct net_device *dev_rx;
+ struct bpf_dtab_netdev *obj;
unsigned int count;
};
@@ -65,23 +68,18 @@ struct bpf_dtab_netdev {
struct bpf_dtab {
struct bpf_map map;
struct bpf_dtab_netdev **netdev_map;
- unsigned long __percpu *flush_needed;
+ struct list_head __percpu *flush_list;
struct list_head list;
};
static DEFINE_SPINLOCK(dev_map_lock);
static LIST_HEAD(dev_map_list);
-static u64 dev_map_bitmap_size(const union bpf_attr *attr)
-{
- return BITS_TO_LONGS((u64) attr->max_entries) * sizeof(unsigned long);
-}
-
static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
{
struct bpf_dtab *dtab;
+ int err, cpu;
u64 cost;
- int err;
if (!capable(CAP_NET_ADMIN))
return ERR_PTR(-EPERM);
@@ -99,7 +97,7 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
/* make sure page count doesn't overflow */
cost = (u64) dtab->map.max_entries * sizeof(struct bpf_dtab_netdev *);
- cost += dev_map_bitmap_size(attr) * num_possible_cpus();
+ cost += sizeof(struct list_head) * num_possible_cpus();
/* if map size is larger than memlock limit, reject it */
err = bpf_map_charge_init(&dtab->map.memory, cost);
@@ -108,28 +106,30 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
err = -ENOMEM;
- /* A per cpu bitfield with a bit per possible net device */
- dtab->flush_needed = __alloc_percpu_gfp(dev_map_bitmap_size(attr),
- __alignof__(unsigned long),
- GFP_KERNEL | __GFP_NOWARN);
- if (!dtab->flush_needed)
+ dtab->flush_list = alloc_percpu(struct list_head);
+ if (!dtab->flush_list)
goto free_charge;
+ for_each_possible_cpu(cpu)
+ INIT_LIST_HEAD(per_cpu_ptr(dtab->flush_list, cpu));
+
dtab->netdev_map = bpf_map_area_alloc(dtab->map.max_entries *
sizeof(struct bpf_dtab_netdev *),
dtab->map.numa_node);
if (!dtab->netdev_map)
- goto free_charge;
+ goto free_percpu;
spin_lock(&dev_map_lock);
list_add_tail_rcu(&dtab->list, &dev_map_list);
spin_unlock(&dev_map_lock);
return &dtab->map;
+
+free_percpu:
+ free_percpu(dtab->flush_list);
free_charge:
bpf_map_charge_finish(&dtab->map.memory);
free_dtab:
- free_percpu(dtab->flush_needed);
kfree(dtab);
return ERR_PTR(err);
}
@@ -158,14 +158,14 @@ static void dev_map_free(struct bpf_map *map)
rcu_barrier();
/* To ensure all pending flush operations have completed wait for flush
- * bitmap to indicate all flush_needed bits to be zero on _all_ cpus.
+ * list to empty on _all_ cpus.
* Because the above synchronize_rcu() ensures the map is disconnected
- * from the program we can assume no new bits will be set.
+ * from the program we can assume no new items will be added.
*/
for_each_online_cpu(cpu) {
- unsigned long *bitmap = per_cpu_ptr(dtab->flush_needed, cpu);
+ struct list_head *flush_list = per_cpu_ptr(dtab->flush_list, cpu);
- while (!bitmap_empty(bitmap, dtab->map.max_entries))
+ while (!list_empty(flush_list))
cond_resched();
}
@@ -181,7 +181,7 @@ static void dev_map_free(struct bpf_map *map)
kfree(dev);
}
- free_percpu(dtab->flush_needed);
+ free_percpu(dtab->flush_list);
bpf_map_area_free(dtab->netdev_map);
kfree(dtab);
}
@@ -203,18 +203,10 @@ static int dev_map_get_next_key(struct bpf_map *map, void *key, void *next_key)
return 0;
}
-void __dev_map_insert_ctx(struct bpf_map *map, u32 bit)
-{
- struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
- unsigned long *bitmap = this_cpu_ptr(dtab->flush_needed);
-
- __set_bit(bit, bitmap);
-}
-
-static int bq_xmit_all(struct bpf_dtab_netdev *obj,
- struct xdp_bulk_queue *bq, u32 flags,
+static int bq_xmit_all(struct xdp_bulk_queue *bq, u32 flags,
bool in_napi_ctx)
{
+ struct bpf_dtab_netdev *obj = bq->obj;
struct net_device *dev = obj->dev;
int sent = 0, drops = 0, err = 0;
int i;
@@ -241,6 +233,8 @@ static int bq_xmit_all(struct bpf_dtab_netdev *obj,
trace_xdp_devmap_xmit(&obj->dtab->map, obj->bit,
sent, drops, bq->dev_rx, dev, err);
bq->dev_rx = NULL;
+ __list_del(bq->flush_node.prev, bq->flush_node.next);
+ bq->flush_node.prev = NULL;
return 0;
error:
/* If ndo_xdp_xmit fails with an errno, no frames have been
@@ -263,31 +257,18 @@ static int bq_xmit_all(struct bpf_dtab_netdev *obj,
* from the driver before returning from its napi->poll() routine. The poll()
* routine is called either from busy_poll context or net_rx_action signaled
* from NET_RX_SOFTIRQ. Either way the poll routine must complete before the
- * net device can be torn down. On devmap tear down we ensure the ctx bitmap
- * is zeroed before completing to ensure all flush operations have completed.
+ * net device can be torn down. On devmap tear down we ensure the flush list
+ * is empty before completing to ensure all flush operations have completed.
*/
void __dev_map_flush(struct bpf_map *map)
{
struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
- unsigned long *bitmap = this_cpu_ptr(dtab->flush_needed);
- u32 bit;
+ struct list_head *flush_list = this_cpu_ptr(dtab->flush_list);
+ struct xdp_bulk_queue *bq, *tmp;
rcu_read_lock();
- for_each_set_bit(bit, bitmap, map->max_entries) {
- struct bpf_dtab_netdev *dev = READ_ONCE(dtab->netdev_map[bit]);
- struct xdp_bulk_queue *bq;
-
- /* This is possible if the dev entry is removed by user space
- * between xdp redirect and flush op.
- */
- if (unlikely(!dev))
- continue;
-
- bq = this_cpu_ptr(dev->bulkq);
- bq_xmit_all(dev, bq, XDP_XMIT_FLUSH, true);
-
- __clear_bit(bit, bitmap);
- }
+ list_for_each_entry_safe(bq, tmp, flush_list, flush_node)
+ bq_xmit_all(bq, XDP_XMIT_FLUSH, true);
rcu_read_unlock();
}
@@ -314,10 +295,11 @@ static int bq_enqueue(struct bpf_dtab_netdev *obj, struct xdp_frame *xdpf,
struct net_device *dev_rx)
{
+ struct list_head *flush_list = this_cpu_ptr(obj->dtab->flush_list);
struct xdp_bulk_queue *bq = this_cpu_ptr(obj->bulkq);
if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
- bq_xmit_all(obj, bq, 0, true);
+ bq_xmit_all(bq, 0, true);
/* Ingress dev_rx will be the same for all xdp_frame's in
* bulk_queue, because bq stored per-CPU and must be flushed
@@ -327,6 +309,10 @@ static int bq_enqueue(struct bpf_dtab_netdev *obj, struct xdp_frame *xdpf,
bq->dev_rx = dev_rx;
bq->q[bq->count++] = xdpf;
+
+ if (!bq->flush_node.prev)
+ list_add(&bq->flush_node, flush_list);
+
return 0;
}
@@ -377,17 +363,12 @@ static void dev_map_flush_old(struct bpf_dtab_netdev *dev)
{
if (dev->dev->netdev_ops->ndo_xdp_xmit) {
struct xdp_bulk_queue *bq;
- unsigned long *bitmap;
-
int cpu;
rcu_read_lock();
for_each_online_cpu(cpu) {
- bitmap = per_cpu_ptr(dev->dtab->flush_needed, cpu);
- __clear_bit(dev->bit, bitmap);
-
bq = per_cpu_ptr(dev->bulkq, cpu);
- bq_xmit_all(dev, bq, XDP_XMIT_FLUSH, false);
+ bq_xmit_all(bq, XDP_XMIT_FLUSH, false);
}
rcu_read_unlock();
}
@@ -434,8 +415,10 @@ static int dev_map_update_elem(struct bpf_map *map, void *key, void *value,
struct net *net = current->nsproxy->net_ns;
gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN;
struct bpf_dtab_netdev *dev, *old_dev;
- u32 i = *(u32 *)key;
u32 ifindex = *(u32 *)value;
+ struct xdp_bulk_queue *bq;
+ u32 i = *(u32 *)key;
+ int cpu;
if (unlikely(map_flags > BPF_EXIST))
return -EINVAL;
@@ -458,6 +441,11 @@ static int dev_map_update_elem(struct bpf_map *map, void *key, void *value,
return -ENOMEM;
}
+ for_each_possible_cpu(cpu) {
+ bq = per_cpu_ptr(dev->bulkq, cpu);
+ bq->obj = dev;
+ }
+
dev->dev = dev_get_by_index(net, ifindex);
if (!dev->dev) {
free_percpu(dev->bulkq);
diff --git a/net/core/filter.c b/net/core/filter.c
index 2014d76e0d2a..183bf4d8e301 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3523,7 +3523,6 @@ static int __bpf_tx_xdp_map(struct net_device *dev_rx, void *fwd,
err = dev_map_enqueue(dst, xdp, dev_rx);
if (unlikely(err))
return err;
- __dev_map_insert_ctx(map, index);
break;
}
case BPF_MAP_TYPE_CPUMAP: {
@@ -3532,7 +3531,6 @@ static int __bpf_tx_xdp_map(struct net_device *dev_rx, void *fwd,
err = cpu_map_enqueue(rcpu, xdp, dev_rx);
if (unlikely(err))
return err;
- __cpu_map_insert_ctx(map, index);
break;
}
case BPF_MAP_TYPE_XSKMAP: {
^ permalink raw reply related
* [PATCH bpf-next v5 3/3] devmap: Allow map lookups from eBPF
From: Toke Høiland-Jørgensen @ 2019-06-23 2:17 UTC (permalink / raw)
To: netdev
Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
David Miller, Jonathan Lemon
In-Reply-To: <156125626076.5209.13424524054109901554.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
We don't currently allow lookups into a devmap from eBPF, because the map
lookup returns a pointer directly to the dev->ifindex, which shouldn't be
modifiable from eBPF.
However, being able to do lookups in devmaps is useful to know (e.g.)
whether forwarding to a specific interface is enabled. Currently, programs
work around this by keeping a shadow map of another type which indicates
whether a map index is valid.
Since we now have a flag to make maps read-only from the eBPF side, we can
simply lift the lookup restriction if we make sure this flag is always set.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
kernel/bpf/devmap.c | 5 +++++
kernel/bpf/verifier.c | 7 ++-----
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 59113bb108c7..f75d992d466d 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -89,6 +89,11 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
return ERR_PTR(-EINVAL);
+ /* Lookup returns a pointer straight to dev->ifindex, so make sure the
+ * verifier prevents writes from the BPF side
+ */
+ attr->map_flags |= BPF_F_RDONLY_PROG;
+
dtab = kzalloc(sizeof(*dtab), GFP_USER);
if (!dtab)
return ERR_PTR(-ENOMEM);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0e079b2298f8..51c327ee7d3f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3407,12 +3407,9 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
if (func_id != BPF_FUNC_get_local_storage)
goto error;
break;
- /* devmap returns a pointer to a live net_device ifindex that we cannot
- * allow to be modified from bpf side. So do not allow lookup elements
- * for now.
- */
case BPF_MAP_TYPE_DEVMAP:
- if (func_id != BPF_FUNC_redirect_map)
+ if (func_id != BPF_FUNC_redirect_map &&
+ func_id != BPF_FUNC_map_lookup_elem)
goto error;
break;
/* Restrict bpf side of cpumap and xskmap, open when use-cases
^ permalink raw reply related
* [PATCH bpf-next v5 0/3] xdp: Allow lookup into devmaps before redirect
From: Toke Høiland-Jørgensen @ 2019-06-23 2:17 UTC (permalink / raw)
To: netdev
Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
David Miller, Jonathan Lemon
When using the bpf_redirect_map() helper to redirect packets from XDP, the eBPF
program cannot currently know whether the redirect will succeed, which makes it
impossible to gracefully handle errors. To properly fix this will probably
require deeper changes to the way TX resources are allocated, but one thing that
is fairly straight forward to fix is to allow lookups into devmaps, so programs
can at least know when a redirect is *guaranteed* to fail because there is no
entry in the map. Currently, programs work around this by keeping a shadow map
of another type which indicates whether a map index is valid.
This series contains two changes that are complementary ways to fix this issue:
- Moving the map lookup into the bpf_redirect_map() helper (and caching the
result), so the helper can return an error if no value is found in the map.
This includes a refactoring of the devmap and cpumap code to not care about
the index on enqueue.
- Allowing regular lookups into devmaps from eBPF programs, using the read-only
flag to make sure they don't change the values.
The performance impact of the series is negligible, in the sense that I cannot
measure it because the variance between test runs is higher than the difference
pre/post series.
Changelog:
v5:
- Rebase on latest bpf-next.
- Update documentation for bpf_redirect_map() with the new meaning of flags.
v4:
- Fix a few nits from Andrii
- Lose the #defines in bpf.h and just compare the flags argument directly to
XDP_TX in bpf_xdp_redirect_map().
v3:
- Adopt Jonathan's idea of using the lower two bits of the flag value as the
return code.
- Always do the lookup, and cache the result for use in xdp_do_redirect(); to
achieve this, refactor the devmap and cpumap code to get rid the bitmap for
selecting which devices to flush.
v2:
- For patch 1, make it clear that the change works for any map type.
- For patch 2, just use the new BPF_F_RDONLY_PROG flag to make the return
value read-only.
---
Toke Høiland-Jørgensen (3):
devmap/cpumap: Use flush list instead of bitmap
bpf_xdp_redirect_map: Perform map lookup in eBPF helper
devmap: Allow map lookups from eBPF
include/linux/filter.h | 1
include/uapi/linux/bpf.h | 7 ++-
kernel/bpf/cpumap.c | 106 ++++++++++++++++++++-----------------------
kernel/bpf/devmap.c | 113 ++++++++++++++++++++++------------------------
kernel/bpf/verifier.c | 7 +--
net/core/filter.c | 29 +++++-------
6 files changed, 123 insertions(+), 140 deletions(-)
^ permalink raw reply
* Re: [PATCH v2 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Willem de Bruijn @ 2019-06-23 2:12 UTC (permalink / raw)
To: Neil Horman; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <20190622174154.14473-1-nhorman@tuxdriver.com>
On Sat, Jun 22, 2019 at 1:42 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> When an application is run that:
> a) Sets its scheduler to be SCHED_FIFO
> and
> b) Opens a memory mapped AF_PACKET socket, and sends frames with the
> MSG_DONTWAIT flag cleared, its possible for the application to hang
> forever in the kernel. This occurs because when waiting, the code in
> tpacket_snd calls schedule, which under normal circumstances allows
> other tasks to run, including ksoftirqd, which in some cases is
> responsible for freeing the transmitted skb (which in AF_PACKET calls a
> destructor that flips the status bit of the transmitted frame back to
> available, allowing the transmitting task to complete).
>
> However, when the calling application is SCHED_FIFO, its priority is
> such that the schedule call immediately places the task back on the cpu,
> preventing ksoftirqd from freeing the skb, which in turn prevents the
> transmitting task from detecting that the transmission is complete.
>
> We can fix this by converting the schedule call to a completion
> mechanism. By using a completion queue, we force the calling task, when
> it detects there are no more frames to send, to schedule itself off the
> cpu until such time as the last transmitted skb is freed, allowing
> forward progress to be made.
>
> Tested by myself and the reporter, with good results
>
> Appies to the net tree
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Matteo Croce <mcroce@redhat.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
>
> Change Notes:
>
> V1->V2:
> Enhance the sleep logic to support being interruptible and
> allowing for honoring to SK_SNDTIMEO (Willem de Bruijn)
> ---
> net/packet/af_packet.c | 60 ++++++++++++++++++++++++++++++++----------
> net/packet/internal.h | 2 ++
> 2 files changed, 48 insertions(+), 14 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index a29d66da7394..8ddb2f7aebb4 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -358,7 +358,8 @@ static inline struct page * __pure pgv_to_page(void *addr)
> return virt_to_page(addr);
> }
>
> -static void __packet_set_status(struct packet_sock *po, void *frame, int status)
> +static void __packet_set_status(struct packet_sock *po, void *frame, int status,
> + bool call_complete)
> {
> union tpacket_uhdr h;
>
> @@ -381,6 +382,8 @@ static void __packet_set_status(struct packet_sock *po, void *frame, int status)
> BUG();
> }
>
> + if (po->wait_on_complete && call_complete)
> + complete(&po->skb_completion);
This wake need not happen before the barrier. Only one caller of
__packet_set_status passes call_complete (tpacket_destruct_skb).
Moving this branch to the caller avoids a lot of code churn.
Also, multiple packets may be released before the process is awoken.
The process will block until packet_read_pending drops to zero. Can
defer the wait_on_complete to that one instance.
> smp_wmb();
> }
>
> @@ -1148,6 +1151,14 @@ static void *packet_previous_frame(struct packet_sock *po,
> return packet_lookup_frame(po, rb, previous, status);
> }
>
> +static void *packet_next_frame(struct packet_sock *po,
> + struct packet_ring_buffer *rb,
> + int status)
> +{
> + unsigned int next = rb->head != rb->frame_max ? rb->head+1 : 0;
> + return packet_lookup_frame(po, rb, next, status);
> +}
> +
> static void packet_increment_head(struct packet_ring_buffer *buff)
> {
> buff->head = buff->head != buff->frame_max ? buff->head+1 : 0;
> @@ -2360,7 +2371,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> #endif
>
> if (po->tp_version <= TPACKET_V2) {
> - __packet_set_status(po, h.raw, status);
> + __packet_set_status(po, h.raw, status, false);
> sk->sk_data_ready(sk);
> } else {
> prb_clear_blk_fill_status(&po->rx_ring);
> @@ -2400,7 +2411,7 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
> packet_dec_pending(&po->tx_ring);
>
> ts = __packet_set_timestamp(po, ph, skb);
> - __packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
> + __packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts, true);
> }
>
> sock_wfree(skb);
> @@ -2585,13 +2596,13 @@ static int tpacket_parse_header(struct packet_sock *po, void *frame,
>
> static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> {
> - struct sk_buff *skb;
> + struct sk_buff *skb = NULL;
> struct net_device *dev;
> struct virtio_net_hdr *vnet_hdr = NULL;
> struct sockcm_cookie sockc;
> __be16 proto;
> int err, reserve = 0;
> - void *ph;
> + void *ph = NULL;
> DECLARE_SOCKADDR(struct sockaddr_ll *, saddr, msg->msg_name);
> bool need_wait = !(msg->msg_flags & MSG_DONTWAIT);
> unsigned char *addr = NULL;
> @@ -2600,9 +2611,12 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> int len_sum = 0;
> int status = TP_STATUS_AVAILABLE;
> int hlen, tlen, copylen = 0;
> + long timeo;
>
> mutex_lock(&po->pg_vec_lock);
>
> + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> +
> if (likely(saddr == NULL)) {
> dev = packet_cached_dev_get(po);
> proto = po->num;
> @@ -2647,16 +2661,29 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> size_max = dev->mtu + reserve + VLAN_HLEN;
>
> do {
> +
> + if (po->wait_on_complete && need_wait) {
> + timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
Why move the sleeping location from where it was with schedule()?
Without that change, ph and skb are both guaranteed to be NULL after
packet_current_frame, so can jump to out_put and avoid adding branches
at out_status. And no need for packet_next_frame.
Just replace schedule with a sleeping function in place (removing the
then obsolete need_resched call).
That is a much shorter patch and easier to compare for correctness
with the current code.
minor: probably preferable to first check local variable need_wait
before reading a struct member.
> + po->wait_on_complete = 0;
> + if (!timeo) {
> + /* We timed out, break out and notify userspace */
> + err = -ETIMEDOUT;
> + goto out_status;
> + } else if (timeo == -ERESTARTSYS) {
> + err = -ERESTARTSYS;
> + goto out_status;
> + }
> + }
> +
> ph = packet_current_frame(po, &po->tx_ring,
> TP_STATUS_SEND_REQUEST);
> - if (unlikely(ph == NULL)) {
> - if (need_wait && need_resched())
> - schedule();
> - continue;
> - }
> +
> + if (likely(ph == NULL))
why switch from unlikely to likely?
> + break;
>
> skb = NULL;
> tp_len = tpacket_parse_header(po, ph, size_max, &data);
> +
nit: irrelevant
> if (tp_len < 0)
> goto tpacket_error;
>
> @@ -2699,7 +2726,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> tpacket_error:
> if (po->tp_loss) {
> __packet_set_status(po, ph,
> - TP_STATUS_AVAILABLE);
> + TP_STATUS_AVAILABLE, false);
> packet_increment_head(&po->tx_ring);
> kfree_skb(skb);
> continue;
> @@ -2719,7 +2746,9 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> }
>
> skb->destructor = tpacket_destruct_skb;
> - __packet_set_status(po, ph, TP_STATUS_SENDING);
> + __packet_set_status(po, ph, TP_STATUS_SENDING, false);
> + if (!packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST))
> + po->wait_on_complete = 1;
> packet_inc_pending(&po->tx_ring);
>
> status = TP_STATUS_SEND_REQUEST;
> @@ -2753,8 +2782,10 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> goto out_put;
>
> out_status:
> - __packet_set_status(po, ph, status);
> - kfree_skb(skb);
> + if (ph)
> + __packet_set_status(po, ph, status, false);
> + if (skb)
> + kfree_skb(skb);
> out_put:
> dev_put(dev);
> out:
> @@ -3207,6 +3238,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol,
> sock_init_data(sock, sk);
>
> po = pkt_sk(sk);
> + init_completion(&po->skb_completion);
> sk->sk_family = PF_PACKET;
> po->num = proto;
> po->xmit = dev_queue_xmit;
> diff --git a/net/packet/internal.h b/net/packet/internal.h
> index 3bb7c5fb3bff..bbb4be2c18e7 100644
> --- a/net/packet/internal.h
> +++ b/net/packet/internal.h
> @@ -128,6 +128,8 @@ struct packet_sock {
> unsigned int tp_hdrlen;
> unsigned int tp_reserve;
> unsigned int tp_tstamp;
> + struct completion skb_completion;
> + unsigned int wait_on_complete;
Probably belong in packet_ring_buffer. Near pending_refcnt as touched
in the same code. And because protected by the ring buffer mutex.
> struct net_device __rcu *cached_dev;
> int (*xmit)(struct sk_buff *skb);
> struct packet_type prot_hook ____cacheline_aligned_in_smp;
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH V34 23/29] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Kees Cook @ 2019-06-23 0:09 UTC (permalink / raw)
To: Matthew Garrett
Cc: jmorris, linux-security-module, linux-kernel, linux-api,
David Howells, Alexei Starovoitov, Matthew Garrett, netdev,
Chun-Yi Lee, Daniel Borkmann
In-Reply-To: <20190622000358.19895-24-matthewgarrett@google.com>
On Fri, Jun 21, 2019 at 05:03:52PM -0700, Matthew Garrett wrote:
> From: David Howells <dhowells@redhat.com>
>
> There are some bpf functions can be used to read kernel memory:
> bpf_probe_read, bpf_probe_write_user and bpf_trace_printk. These allow
> private keys in kernel memory (e.g. the hibernation image signing key) to
> be read by an eBPF program and kernel memory to be altered without
> restriction. Disable them if the kernel has been locked down in
> confidentiality mode.
>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> cc: netdev@vger.kernel.org
> cc: Chun-Yi Lee <jlee@suse.com>
> cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> ---
> include/linux/security.h | 1 +
> kernel/trace/bpf_trace.c | 20 +++++++++++++++++++-
> security/lockdown/lockdown.c | 1 +
> 3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index e6e3e2403474..de0d37b1fe79 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -97,6 +97,7 @@ enum lockdown_reason {
> LOCKDOWN_INTEGRITY_MAX,
> LOCKDOWN_KCORE,
> LOCKDOWN_KPROBES,
> + LOCKDOWN_BPF_READ,
> LOCKDOWN_CONFIDENTIALITY_MAX,
> };
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d64c00afceb5..638f9b00a8df 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -137,6 +137,10 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
> {
> int ret;
>
> + ret = security_locked_down(LOCKDOWN_BPF_READ);
> + if (ret)
> + return ret;
> +
> ret = probe_kernel_read(dst, unsafe_ptr, size);
> if (unlikely(ret < 0))
> memset(dst, 0, size);
> @@ -156,6 +160,12 @@ static const struct bpf_func_proto bpf_probe_read_proto = {
> BPF_CALL_3(bpf_probe_write_user, void *, unsafe_ptr, const void *, src,
> u32, size)
> {
> + int ret;
> +
> + ret = security_locked_down(LOCKDOWN_BPF_READ);
> + if (ret)
> + return ret;
> +
> /*
> * Ensure we're in user context which is safe for the helper to
> * run. This helper has no business in a kthread.
> @@ -205,7 +215,11 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
> int fmt_cnt = 0;
> u64 unsafe_addr;
> char buf[64];
> - int i;
> + int i, ret;
> +
> + ret = security_locked_down(LOCKDOWN_BPF_READ);
> + if (ret)
> + return ret;
>
> /*
> * bpf_check()->check_func_arg()->check_stack_boundary()
> @@ -534,6 +548,10 @@ BPF_CALL_3(bpf_probe_read_str, void *, dst, u32, size,
> {
> int ret;
>
> + ret = security_locked_down(LOCKDOWN_BPF_READ);
> + if (ret)
> + return ret;
> +
> /*
> * The strncpy_from_unsafe() call will likely not fill the entire
> * buffer, but that's okay in this circumstance as we're probing
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index 5a08c17f224d..2eea2cc13117 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -33,6 +33,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> [LOCKDOWN_INTEGRITY_MAX] = "integrity",
> [LOCKDOWN_KCORE] = "/proc/kcore access",
> [LOCKDOWN_KPROBES] = "use of kprobes",
> + [LOCKDOWN_BPF_READ] = "use of bpf to read kernel RAM",
> [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
> };
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH net] ipv6: fix neighbour resolution with raw socket
From: David Miller @ 2019-06-23 0:08 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev
In-Reply-To: <20190622.170712.2073255657139689928.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Sat, 22 Jun 2019 17:07:12 -0700 (PDT)
> Applied and queued up for -stable, thanks.
Actually, this needs a warning fix in bluetooth and netfilter. Please
fix this up, do a proper allmodconfig build, and resubmit.
net/bluetooth/6lowpan.c: In function ‘peer_lookup_dst’:
net/bluetooth/6lowpan.c:188:11: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
net/netfilter/nf_flow_table_ip.c: In function ‘nf_flow_offload_ipv6_hook’:
net/netfilter/nf_flow_table_ip.c:481:10: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
Thank you.
^ 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