Re: [PATCH net] net/xen-netback: prevent UAF in xenvif_flush_hash()
On Wed, 28 Aug 2024 21:52:12 +0900 Jeongjun Park wrote: > > The loop runs with irq disabled, the RCU critical section extends over > > it, uninterrupted. > > Basically, list_for_each_entry_rcu is specified to be used under the > protection > of rcu_read_lock(), but this is not the case with xenvif_new_hash(). If it is > used without the protection of rcu_read_lock(), kfree is called immediately > after the grace period ends after the call to kfree_rcu() inside > list_for_each_entry_rcu, so the entry is released, and a UAF occurs when > fetching with ->next thereafter. You cut off and didn't answer Paolo's question whether you have a splat / saw this actually cause a crash or a KASAN warning.
Re: [PATCH net] net/xen-netback: prevent UAF in xenvif_flush_hash()
On Tue, 27 Aug 2024 13:19:59 +0200 Paolo Abeni wrote: > On 8/22/24 20:11, Jeongjun Park wrote: > > During the list_for_each_entry_rcu iteration call of xenvif_flush_hash, > > kfree_rcu does not exist inside the rcu read critical section, so if > > The above wording is confusing, do you mean "kfree_rcu does not exit > from "...? I think they mean that kfree_rcu() is called without holding RCU read lock.. > > kfree_rcu is called when the rcu grace period ends during the iteration, > > UAF occurs when accessing head->next after the entry becomes free. .. so it can run immediately. Therefore the loop fetching head->next may cause a UAF. > The loop runs with irq disabled, the RCU critical section extends over > it, uninterrupted. Is this an official RCU rule? I remember Paul told us it's the case for softirq, but IDK if it is also for local IRQ disable. > Do you have a splat for the reported UAF? > > This does not look the correct solution. The problem may not exist, but FWIW the change makes sense to me :) We hold the write lock, and modify the list. for_each_entry_safe() seems like a better fit than for_each_entry_rcu()
Re: [PATCH] xen-netback: Use seq_putc() in xenvif_dump_hash_info()
On Mon, 15 Jul 2024 22:24:39 +0200 Christophe JAILLET wrote: > Most of the time, this kind of modification is useless because it is > already done by the compiler, see [1]. GTK, thanks!
Re: [PATCH] xen-netback: Use seq_putc() in xenvif_dump_hash_info()
On Sat, 13 Jul 2024 15:18:42 +0200 Markus Elfring wrote: > Single characters (line breaks) should be put into a sequence. > Thus use the corresponding function “seq_putc”. > > This issue was transformed by using the Coccinelle software. I prefer to only merge trivial changes like this if maintainer indicates their support by acking them. Since the merge window has opened we can't wait and see so I'm marking this patch and your pktgen patch as deferred.
Re: [PATCH net] xen-netback: properly sync TX responses
On Mon, 29 Jan 2024 14:03:08 +0100 Jan Beulich wrote:
> Invoking the make_tx_response() / push_tx_responses() pair with no lock
> held would be acceptable only if all such invocations happened from the
> same context (NAPI instance or dealloc thread). Since this isn't the
> case, and since the interface "spec" also doesn't demand that multicast
> operations may only be performed with no in-flight transmits,
> MCAST_{ADD,DEL} processing also needs to acquire the response lock
> around the invocations.
>
> To prevent similar mistakes going forward, "downgrade" the present
> functions to private helpers of just the two remaining ones using them
> directly, with no forward declarations anymore. This involves renaming
> what so far was make_tx_response(), for the new function of that name
> to serve the new (wrapper) purpose.
>
> While there,
> - constify the txp parameters,
> - correct xenvif_idx_release()'s status parameter's type,
> - rename {,_}make_tx_response()'s status parameters for consistency with
> xenvif_idx_release()'s.
Hi Paul, is this one on your TODO list to review or should
we do our best? :)
--
pw-bot: needs-ack
Re: [PATCH] net/xen-netback: Break build if netback slots > max_skbs + 1
On Thu, 5 Oct 2023 18:39:51 +0300 David Kahurani wrote: > > MAX_SKB_FRAGS can now be set via Kconfig, this allows us to create > > larger super-packets. Can XEN_NETBK_LEGACY_SLOTS_MAX be made relative > > to MAX_SKB_FRAGS, or does the number have to match between guest and > > host? > > Historically, netback driver allows for a maximum of 18 fragments. > With recent changes, it also relies on the assumption that the > difference between MAX_SKB_FRAGS and XEN_NETBK_LEGACY_SLOTS_MAX is one > and MAX_SKB_FRAGS is the lesser value. > > Now, look at Ubuntu kernel for instance( a change has been made and, > presumably, with good reason so we have reason to assume that the > change will persist in future releases). > > /* To allow 64K frame to be packed as single skb without frag_list we > * require 64K/PAGE_SIZE pages plus 1 additional page to allow for > * buffers which do not start on a page boundary. > * > * Since GRO uses frags we allocate at least 16 regardless of page > * size. > */ > #if (65536/PAGE_SIZE + 1) < 16 > #define MAX_SKB_FRAGS 16UL > #else > #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1) > #endif > > So, MAX_SKB_FRAGS can sometimes be 16. This is exactly what we're > trying to avoid with this patch. I host running with this change is > vulnerable to attack by the guest(though, this will only happen when > PAGE_SIZE > 4096). My bad, you're protecting from the inverse condition than I thought. But to be clear the code you're quoting (the defines for MAX_SKB_FRAGS) are what has been there upstream forever until 3948b059 was merged. Not 100% sure why 3948b059 switched the min from 16 to 17, I think it was just to keep consistency between builds. If this change gets backported to 6.1 stable it will break ppc build of stable, right? Since ppc has 64k pages.
Re: [PATCH] net/xen-netback: Break build if netback slots > max_skbs + 1
On Wed, 27 Sep 2023 11:29:18 +0300 David Kahurani wrote: > If XEN_NETBK_LEGACY_SLOTS_MAX and MAX_SKB_FRAGS have a difference of > more than 1, with MAX_SKB_FRAGS being the lesser value, it opens up a > path for null-dereference. It was also noted that some distributions > were modifying upstream behaviour in that direction which necessitates > this patch. MAX_SKB_FRAGS can now be set via Kconfig, this allows us to create larger super-packets. Can XEN_NETBK_LEGACY_SLOTS_MAX be made relative to MAX_SKB_FRAGS, or does the number have to match between guest and host? Option #2 would be to add a Kconfig dependency for the driver to make sure high MAX_SKB_FRAGS is incompatible with it. Breaking the build will make build bots very sad. We'll also need a Fixes tag, I presume this is a fix? -- pw-bot: cr
Re: [PATCH] xen/netback: Pass (void *) to virt_to_page()
On Wed, 24 May 2023 22:11:47 -0700 Jakub Kicinski wrote: > On Tue, 23 May 2023 16:03:42 +0200 Linus Walleij wrote: > > virt_to_page() takes a virtual address as argument but > > the driver passes an unsigned long, which works because > > the target platform(s) uses polymorphic macros to calculate > > the page. > > > > Since many architectures implement virt_to_pfn() as > > a macro, this function becomes polymorphic and accepts both a > > (unsigned long) and a (void *). > > > > Fix this up by an explicit (void *) cast. > > Paul, Wei, looks like netdev may be the usual path for this patch > to flow thru, although I'm never 100% sure with Xen. > Please ack or LUK if you prefer to direct the patch elsewhere? Ugh, Wei already acked this, sorry for the noise.
Re: [PATCH] xen/netback: Pass (void *) to virt_to_page()
On Tue, 23 May 2023 16:03:42 +0200 Linus Walleij wrote: > virt_to_page() takes a virtual address as argument but > the driver passes an unsigned long, which works because > the target platform(s) uses polymorphic macros to calculate > the page. > > Since many architectures implement virt_to_pfn() as > a macro, this function becomes polymorphic and accepts both a > (unsigned long) and a (void *). > > Fix this up by an explicit (void *) cast. Paul, Wei, looks like netdev may be the usual path for this patch to flow thru, although I'm never 100% sure with Xen. Please ack or LUK if you prefer to direct the patch elsewhere?
Re: [PATCH] xen-netback: remove unused variables pending_idx and index
On Mon, 27 Feb 2023 09:29:30 +0100 Juergen Gross wrote: > On 26.02.23 17:34, Tom Rix wrote: > > building with gcc and W=1 reports > > drivers/net/xen-netback/netback.c:886:21: error: variable > >‘pending_idx’ set but not used [-Werror=unused-but-set-variable] > >886 | u16 pending_idx; > >| ^~~ > > > > pending_idx is not used so remove it. Since index was only > > used to set pending_idx, remove index as well. > > > > Signed-off-by: Tom Rix > > Reviewed-by: Juergen Gross Applied, thanks!
Re: [PATCH] drivers/net/netfront: Fix NULL sring after live migration
On Tue, 29 Nov 2022 06:17:02 + Lin Liu wrote: > A NAPI is setup for each network sring to poll data to kernel > The sring with source host is destroyed before live migration and > new sring with target host is setup after live migration. > The NAPI for the old sring is not deleted until setup new sring > with target host after migration. With busy_poll/busy_read enabled, > the NAPI can be polled before got deleted when resume VM. > > [50116.602938] BUG: unable to handle kernel NULL pointer dereference at > 0008 > [50116.603047] IP: xennet_poll+0xae/0xd20 > [50116.603090] PGD 0 P4D 0 > [50116.603118] Oops: [#1] SMP PTI > [50116.604624] Call Trace: > [50116.604674] ? finish_task_switch+0x71/0x230 > [50116.604745] ? timerqueue_del+0x1d/0x40 > [50116.604807] ? hrtimer_try_to_cancel+0xb5/0x110 > [50116.604882] ? xennet_alloc_rx_buffers+0x2a0/0x2a0 > [50116.604958] napi_busy_loop+0xdb/0x270 > [50116.605017] sock_poll+0x87/0x90 > [50116.605066] do_sys_poll+0x26f/0x580 > [50116.605125] ? tracing_map_insert+0x1d4/0x2f0 > [50116.605196] ? event_hist_trigger+0x14a/0x260 You can trim all the ' ? ' entries from the stack trace, and the time stamps, FWIW. Makes it easier to read. > [50116.613598] ? finish_task_switch+0x71/0x230 > [50116.614131] ? __schedule+0x256/0x890 > [50116.614640] ? recalc_sigpending+0x1b/0x50 > [50116.615144] ? xen_sched_clock+0x15/0x20 > [50116.615643] ? __rb_reserve_next+0x12d/0x140 > [50116.616138] ? ring_buffer_lock_reserve+0x123/0x3d0 > [50116.616634] ? event_triggers_call+0x87/0xb0 > [50116.617138] ? trace_event_buffer_commit+0x1c4/0x210 > [50116.617625] ? xen_clocksource_get_cycles+0x15/0x20 > [50116.618112] ? ktime_get_ts64+0x51/0xf0 > [50116.618578] SyS_ppoll+0x160/0x1a0 > [50116.619029] ? SyS_ppoll+0x160/0x1a0 > [50116.619475] do_syscall_64+0x73/0x130 > [50116.619901] entry_SYSCALL_64_after_hwframe+0x41/0xa6 > ... > [50116.806230] RIP: xennet_poll+0xae/0xd20 RSP: b4f041933900 > [50116.806772] CR2: 0008 > [50116.807337] ---[ end trace f8601785b354351c ]--- > > xen frontend should remove the NAPIs for the old srings before live > migration as the bond srings are destroyed > > There is a tiny window between the srings are set to NULL and > the NAPIs are disabled, It is safe as the NAPI threads are still > frozen at that time > Since this is a fix please add a Fixes tag, and add [PATCH net] to the subject. > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 9af2b027c19c..dc404e05970c 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -1862,6 +1862,12 @@ static int netfront_resume(struct xenbus_device *dev) > netif_tx_unlock_bh(info->netdev); > > xennet_disconnect_backend(info); > + > + rtnl_lock(); > + if (info->queues) > + xennet_destroy_queues(info); > + rtnl_unlock(); Now all callers of xennet_disconnect_backend() destroy queues soon after, can we just move the destroy queues into disconnect ? > return 0; > } >
Re: [PATCH Resend] xen/netback: do some code cleanup
On Tue, 7 Jun 2022 07:28:38 +0200 Juergen Gross wrote:
> Remove some unused macros and functions, make local functions static.
> --- a/drivers/net/xen-netback/rx.c
> +++ b/drivers/net/xen-netback/rx.c
> @@ -486,7 +486,7 @@ static void xenvif_rx_skb(struct xenvif_queue *queue)
>#define RX_BATCH_SIZE 64
> -void xenvif_rx_action(struct xenvif_queue *queue)
> +static void xenvif_rx_action(struct xenvif_queue *queue)
Strange, I haven't seen this kind of corruption before, but the patch
certainly looks corrupted. It doesn't apply.
Could you "git send-email" it?
> {
Re: [PATCH] xen/netback: do some code cleanup
On Mon, 30 May 2022 13:41:03 +0200 Juergen Gross wrote: > Remove some unused macros and functions, make local functions static. > > Signed-off-by: Juergen Gross # Form letter - net-next is closed We have already sent the networking pull request for 5.19 and therefore net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only. Please repost when net-next reopens after 5.19-rc1 is cut. RFC patches sent for review only are obviously welcome at any time.
[PATCH net-next v2 01/15] eth: remove copies of the NAPI_POLL_WEIGHT define
Defining local versions of NAPI_POLL_WEIGHT with the same
values in the drivers just makes refactoring harder.
Drop the special defines in a bunch of drivers where the
removal is relatively simple so grouping into one patch
does not impact reviewability.
Signed-off-by: Jakub Kicinski
---
CC: ulli.kr...@googlemail.com
CC: linus.wall...@linaro.org
CC: mlind...@marvell.com
CC: step...@networkplumber.org
CC: n...@nbd.name
CC: j...@phrozen.org
CC: sean.w...@mediatek.com
CC: mark-mc@mediatek.com
CC: matthias@gmail.com
CC: grygorii.stras...@ti.com
CC: wei@kernel.org
CC: p...@xen.org
CC: prabhakar.mahadev-lad...@bp.renesas.com
CC: linux-arm-ker...@lists.infradead.org
CC: linux-media...@lists.infradead.org
CC: linux-o...@vger.kernel.org
CC: xen-devel@lists.xenproject.org
---
drivers/net/ethernet/cortina/gemini.c | 4 +---
drivers/net/ethernet/marvell/skge.c | 3 +--
drivers/net/ethernet/marvell/sky2.c | 3 +--
drivers/net/ethernet/mediatek/mtk_star_emac.c | 3 +--
drivers/net/ethernet/ti/davinci_emac.c| 3 +--
drivers/net/ethernet/ti/netcp_core.c | 5 ++---
drivers/net/xen-netback/interface.c | 3 +--
7 files changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c
b/drivers/net/ethernet/cortina/gemini.c
index 8014eb33937c..9e6de2f968fa 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -68,7 +68,6 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
#define DEFAULT_GMAC_RXQ_ORDER 9
#define DEFAULT_GMAC_TXQ_ORDER 8
#define DEFAULT_RX_BUF_ORDER 11
-#define DEFAULT_NAPI_WEIGHT64
#define TX_MAX_FRAGS 16
#define TX_QUEUE_NUM 1 /* max: 6 */
#define RX_MAX_ALLOC_ORDER 2
@@ -2472,8 +2471,7 @@ static int gemini_ethernet_port_probe(struct
platform_device *pdev)
netdev->max_mtu = 10236 - VLAN_ETH_HLEN;
port->freeq_refill = 0;
- netif_napi_add(netdev, &port->napi, gmac_napi_poll,
- DEFAULT_NAPI_WEIGHT);
+ netif_napi_add(netdev, &port->napi, gmac_napi_poll, NAPI_POLL_WEIGHT);
ret = of_get_mac_address(np, mac);
if (!ret) {
diff --git a/drivers/net/ethernet/marvell/skge.c
b/drivers/net/ethernet/marvell/skge.c
index cf03c67fbf40..c1e985416c0e 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -50,7 +50,6 @@
#define PHY_RETRIES1000
#define ETH_JUMBO_MTU 9000
#define TX_WATCHDOG(5 * HZ)
-#define NAPI_WEIGHT64
#define BLINK_MS 250
#define LINK_HZHZ
@@ -3833,7 +3832,7 @@ static struct net_device *skge_devinit(struct skge_hw
*hw, int port,
dev->features |= NETIF_F_HIGHDMA;
skge = netdev_priv(dev);
- netif_napi_add(dev, &skge->napi, skge_poll, NAPI_WEIGHT);
+ netif_napi_add(dev, &skge->napi, skge_poll, NAPI_POLL_WEIGHT);
skge->netdev = dev;
skge->hw = hw;
skge->msg_enable = netif_msg_init(debug, default_msg);
diff --git a/drivers/net/ethernet/marvell/sky2.c
b/drivers/net/ethernet/marvell/sky2.c
index ea16b1dd6a98..a1e907c85217 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -63,7 +63,6 @@
#define TX_DEF_PENDING 63
#define TX_WATCHDOG(5 * HZ)
-#define NAPI_WEIGHT64
#define PHY_RETRIES1000
#define SKY2_EEPROM_MAGIC 0x9955aabb
@@ -4938,7 +4937,7 @@ static int sky2_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
}
}
- netif_napi_add(dev, &hw->napi, sky2_poll, NAPI_WEIGHT);
+ netif_napi_add(dev, &hw->napi, sky2_poll, NAPI_POLL_WEIGHT);
err = register_netdev(dev);
if (err) {
diff --git a/drivers/net/ethernet/mediatek/mtk_star_emac.c
b/drivers/net/ethernet/mediatek/mtk_star_emac.c
index 4cd0747edaff..95839fd84dab 100644
--- a/drivers/net/ethernet/mediatek/mtk_star_emac.c
+++ b/drivers/net/ethernet/mediatek/mtk_star_emac.c
@@ -30,7 +30,6 @@
#define MTK_STAR_WAIT_TIMEOUT 300
#define MTK_STAR_MAX_FRAME_SIZE1514
#define MTK_STAR_SKB_ALIGNMENT 16
-#define MTK_STAR_NAPI_WEIGHT 64
#define MTK_STAR_HASHTABLE_MC_LIMIT256
#define MTK_STAR_HASHTABLE_SIZE_MAX512
@@ -1551,7 +1550,7 @@ static int mtk_star_probe(struct platform_device *pdev)
ndev->netdev_ops = &mtk_star_netdev_ops;
ndev->ethtool_ops = &mtk_star_ethtool_ops;
- netif_napi_add(ndev, &priv->napi, mtk_star_poll, MTK_STAR_NAPI_WEIGHT);
+ netif_napi_add(ndev, &priv->napi, mtk_star_poll, NAPI_POLL_WEIGHT);
return devm_register_netdev(dev, ndev);
}
diff --git a/d
[PATCH net-next 01/14] eth: remove copies of the NAPI_POLL_WEIGHT define
Defining local versions of NAPI_POLL_WEIGHT with the same
values in the drivers just makes refactoring harder.
Drop the special defines in a bunch of drivers where the
removal is relatively simple so grouping into one patch
does not impact reviewability.
Signed-off-by: Jakub Kicinski
---
CC: ulli.kr...@googlemail.com
CC: linus.wall...@linaro.org
CC: mlind...@marvell.com
CC: step...@networkplumber.org
CC: n...@nbd.name
CC: j...@phrozen.org
CC: sean.w...@mediatek.com
CC: mark-mc@mediatek.com
CC: matthias@gmail.com
CC: grygorii.stras...@ti.com
CC: wei@kernel.org
CC: p...@xen.org
CC: prabhakar.mahadev-lad...@bp.renesas.com
CC: linux-arm-ker...@lists.infradead.org
CC: linux-media...@lists.infradead.org
CC: linux-o...@vger.kernel.org
CC: xen-devel@lists.xenproject.org
---
drivers/net/ethernet/cortina/gemini.c | 4 +---
drivers/net/ethernet/marvell/skge.c | 3 +--
drivers/net/ethernet/marvell/sky2.c | 3 +--
drivers/net/ethernet/mediatek/mtk_star_emac.c | 3 +--
drivers/net/ethernet/ti/davinci_emac.c| 3 +--
drivers/net/ethernet/ti/netcp_core.c | 5 ++---
drivers/net/xen-netback/interface.c | 3 +--
7 files changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c
b/drivers/net/ethernet/cortina/gemini.c
index 8014eb33937c..9e6de2f968fa 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -68,7 +68,6 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
#define DEFAULT_GMAC_RXQ_ORDER 9
#define DEFAULT_GMAC_TXQ_ORDER 8
#define DEFAULT_RX_BUF_ORDER 11
-#define DEFAULT_NAPI_WEIGHT64
#define TX_MAX_FRAGS 16
#define TX_QUEUE_NUM 1 /* max: 6 */
#define RX_MAX_ALLOC_ORDER 2
@@ -2472,8 +2471,7 @@ static int gemini_ethernet_port_probe(struct
platform_device *pdev)
netdev->max_mtu = 10236 - VLAN_ETH_HLEN;
port->freeq_refill = 0;
- netif_napi_add(netdev, &port->napi, gmac_napi_poll,
- DEFAULT_NAPI_WEIGHT);
+ netif_napi_add(netdev, &port->napi, gmac_napi_poll, NAPI_POLL_WEIGHT);
ret = of_get_mac_address(np, mac);
if (!ret) {
diff --git a/drivers/net/ethernet/marvell/skge.c
b/drivers/net/ethernet/marvell/skge.c
index cf03c67fbf40..c1e985416c0e 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -50,7 +50,6 @@
#define PHY_RETRIES1000
#define ETH_JUMBO_MTU 9000
#define TX_WATCHDOG(5 * HZ)
-#define NAPI_WEIGHT64
#define BLINK_MS 250
#define LINK_HZHZ
@@ -3833,7 +3832,7 @@ static struct net_device *skge_devinit(struct skge_hw
*hw, int port,
dev->features |= NETIF_F_HIGHDMA;
skge = netdev_priv(dev);
- netif_napi_add(dev, &skge->napi, skge_poll, NAPI_WEIGHT);
+ netif_napi_add(dev, &skge->napi, skge_poll, NAPI_POLL_WEIGHT);
skge->netdev = dev;
skge->hw = hw;
skge->msg_enable = netif_msg_init(debug, default_msg);
diff --git a/drivers/net/ethernet/marvell/sky2.c
b/drivers/net/ethernet/marvell/sky2.c
index ea16b1dd6a98..a1e907c85217 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -63,7 +63,6 @@
#define TX_DEF_PENDING 63
#define TX_WATCHDOG(5 * HZ)
-#define NAPI_WEIGHT64
#define PHY_RETRIES1000
#define SKY2_EEPROM_MAGIC 0x9955aabb
@@ -4938,7 +4937,7 @@ static int sky2_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
}
}
- netif_napi_add(dev, &hw->napi, sky2_poll, NAPI_WEIGHT);
+ netif_napi_add(dev, &hw->napi, sky2_poll, NAPI_POLL_WEIGHT);
err = register_netdev(dev);
if (err) {
diff --git a/drivers/net/ethernet/mediatek/mtk_star_emac.c
b/drivers/net/ethernet/mediatek/mtk_star_emac.c
index 4cd0747edaff..95839fd84dab 100644
--- a/drivers/net/ethernet/mediatek/mtk_star_emac.c
+++ b/drivers/net/ethernet/mediatek/mtk_star_emac.c
@@ -30,7 +30,6 @@
#define MTK_STAR_WAIT_TIMEOUT 300
#define MTK_STAR_MAX_FRAME_SIZE1514
#define MTK_STAR_SKB_ALIGNMENT 16
-#define MTK_STAR_NAPI_WEIGHT 64
#define MTK_STAR_HASHTABLE_MC_LIMIT256
#define MTK_STAR_HASHTABLE_SIZE_MAX512
@@ -1551,7 +1550,7 @@ static int mtk_star_probe(struct platform_device *pdev)
ndev->netdev_ops = &mtk_star_netdev_ops;
ndev->ethtool_ops = &mtk_star_ethtool_ops;
- netif_napi_add(ndev, &priv->napi, mtk_star_poll, MTK_STAR_NAPI_WEIGHT);
+ netif_napi_add(ndev, &priv->napi, mtk_star_poll, NAPI_POLL_WEIGHT);
return devm_register_netdev(dev, ndev);
}
diff --git a/d
Re: [PATCH v2 1/2] Revert "xen-netback: remove 'hotplug-status' once it has served its purpose"
On Tue, 22 Feb 2022 01:18:16 +0100 Marek Marczykowski-Górecki wrote: > This reverts commit 1f2565780e9b7218cf92c7630130e82dcc0fe9c2. > > The 'hotplug-status' node should not be removed as long as the vif > device remains configured. Otherwise the xen-netback would wait for > re-running the network script even if it was already called (in case of > the frontent re-connecting). But also, it _should_ be removed when the > vif device is destroyed (for example when unbinding the driver) - > otherwise hotplug script would not configure the device whenever it > re-appear. > > Moving removal of the 'hotplug-status' node was a workaround for nothing > calling network script after xen-netback module is reloaded. But when > vif interface is re-created (on xen-netback unbind/bind for example), > the script should be called, regardless of who does that - currently > this case is not handled by the toolstack, and requires manual > script call. Keeping hotplug-status=connected to skip the call is wrong > and leads to not configured interface. > > More discussion at > https://lore.kernel.org/xen-devel/afedd7cb-a291-e773-8b0d-4db9b291f...@ipxe.org/T/#u > > Cc: sta...@vger.kernel.org > Signed-off-by: Marek Marczykowski-Górecki Wei, Paul, do these look good?
Re: [PATCH] xen/netfront: destroy queues before real_num_tx_queues is zeroed
On Mon, 21 Feb 2022 07:27:32 +0100 Juergen Gross wrote:
> On 20.02.22 14:42, Marek Marczykowski-Górecki wrote:
> > xennet_destroy_queues() relies on info->netdev->real_num_tx_queues to
> > delete queues. Since d7dac083414eb5bb99a6d2ed53dc2c1b405224e5
> > ("net-sysfs: update the queue counts in the unregistration path"),
> > unregister_netdev() indirectly sets real_num_tx_queues to 0. Those two
> > facts together means, that xennet_destroy_queues() called from
> > xennet_remove() cannot do its job, because it's called after
> > unregister_netdev(). This results in kfree-ing queues that are still
> > linked in napi, which ultimately crashes:
> >
> > BUG: kernel NULL pointer dereference, address:
> > #PF: supervisor read access in kernel mode
> > #PF: error_code(0x) - not-present page
> > PGD 0 P4D 0
> > Oops: [#1] PREEMPT SMP PTI
> > CPU: 1 PID: 52 Comm: xenwatch Tainted: GW
> > 5.16.10-1.32.fc32.qubes.x86_64+ #226
> > RIP: 0010:free_netdev+0xa3/0x1a0
> > Code: ff 48 89 df e8 2e e9 00 00 48 8b 43 50 48 8b 08 48 8d b8 a0 fe
> > ff ff 48 8d a9 a0 fe ff ff 49 39 c4 75 26 eb 47 e8 ed c1 66 ff <48> 8b 85
> > 60 01 00 00 48 8d 95 60 01 00 00 48 89 ef 48 2d 60 01 00
> > RSP: :c9bcfd00 EFLAGS: 00010286
> > RAX: RBX: 88800edad000 RCX:
> > RDX: 0001 RSI: c9bcfc30 RDI:
> > RBP: fea0 R08: R09:
> > R10: R11: 0001 R12: 88800edad050
> > R13: 8880065f8f88 R14: R15: 8880066c6680
> > FS: () GS:8880f330()
> > knlGS:
> > CS: 0010 DS: ES: CR0: 80050033
> > CR2: CR3: e998c006 CR4: 003706e0
> > Call Trace:
> >
> > xennet_remove+0x13d/0x300 [xen_netfront]
> > xenbus_dev_remove+0x6d/0xf0
> > __device_release_driver+0x17a/0x240
> > device_release_driver+0x24/0x30
> > bus_remove_device+0xd8/0x140
> > device_del+0x18b/0x410
> > ? _raw_spin_unlock+0x16/0x30
> > ? klist_iter_exit+0x14/0x20
> > ? xenbus_dev_request_and_reply+0x80/0x80
> > device_unregister+0x13/0x60
> > xenbus_dev_changed+0x18e/0x1f0
> > xenwatch_thread+0xc0/0x1a0
> > ? do_wait_intr_irq+0xa0/0xa0
> > kthread+0x16b/0x190
> > ? set_kthread_struct+0x40/0x40
> > ret_from_fork+0x22/0x30
> >
> >
> > Fix this by calling xennet_destroy_queues() from xennet_close() too,
> > when real_num_tx_queues is still available. This ensures that queues are
> > destroyed when real_num_tx_queues is set to 0, regardless of how
> > unregister_netdev() was called.
> >
> > Originally reported at
> > https://github.com/QubesOS/qubes-issues/issues/7257
> >
> > Fixes: d7dac083414eb5bb9 ("net-sysfs: update the queue counts in the
> > unregistration path")
> > Cc: sta...@vger.kernel.org # 5.16+
> > Signed-off-by: Marek Marczykowski-Górecki
> >
> > ---
> > While this fixes the issue, I'm not sure if that is the correct thing
> > to do. xennet_remove() calls xennet_destroy_queues() under rtnl_lock,
> > which may be important here? Just moving xennet_destroy_queues() before
>
> I checked some of the call paths leading to xennet_close(), and all of
> those contained an ASSERT_RTNL(), so it seems the rtnl_lock is already
> taken here. Could you test with adding an ASSERT_RTNL() in
> xennet_destroy_queues()?
>
> > unregister_netdev() in xennet_remove() did not helped - it crashed in
> > another way (use-after-free in xennet_close()).
>
> Yes, this would need to basically do the xennet_close() handling in
> xennet_destroy() instead, which I believe is not really an option.
I think the patch makes open/close asymmetric, tho. After ifup ; ifdown;
the next ifup will fail because queues are already destroyed, no?
IOW xennet_open() expects the queues were created at an earlier stage.
Maybe we can move the destroy to ndo_uninit? (and create to ndo_init?)
[PATCH net-next v2 01/12] net: xen: use eth_hw_addr_set()
Commit 406f42fa0d3c ("net-next: When a bond have a massive amount
of VLANs...") introduced a rbtree for faster Ethernet address look
up. To maintain netdev->dev_addr in this tree we need to make all
the writes to it got through appropriate helpers.
Signed-off-by: Jakub Kicinski
---
CC: wei@kernel.org
CC: p...@xen.org
CC: boris.ostrov...@oracle.com
CC: jgr...@suse.com
CC: sstabell...@kernel.org
CC: xen-devel@lists.xenproject.org
---
drivers/net/xen-netback/interface.c | 6 --
drivers/net/xen-netfront.c | 4 +++-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/xen-netback/interface.c
b/drivers/net/xen-netback/interface.c
index c58996c1e230..fe8e21ad8ed9 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -494,6 +494,9 @@ static const struct net_device_ops xenvif_netdev_ops = {
struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
unsigned int handle)
{
+ static const u8 dummy_addr[ETH_ALEN] = {
+ 0xfe, 0xff, 0xff, 0xff, 0xff, 0xff,
+ };
int err;
struct net_device *dev;
struct xenvif *vif;
@@ -551,8 +554,7 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t
domid,
* stolen by an Ethernet bridge for STP purposes.
* (FE:FF:FF:FF:FF:FF)
*/
- eth_broadcast_addr(dev->dev_addr);
- dev->dev_addr[0] &= ~0x01;
+ eth_hw_addr_set(dev, dummy_addr);
netif_carrier_off(dev);
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e31b98403f31..57437e4b8a94 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -2157,6 +2157,7 @@ static int talk_to_netback(struct xenbus_device *dev,
unsigned int max_queues = 0;
struct netfront_queue *queue = NULL;
unsigned int num_queues = 1;
+ u8 addr[ETH_ALEN];
info->netdev->irq = 0;
@@ -2170,11 +2171,12 @@ static int talk_to_netback(struct xenbus_device *dev,
"feature-split-event-channels", 0);
/* Read mac addr. */
- err = xen_net_read_mac(dev, info->netdev->dev_addr);
+ err = xen_net_read_mac(dev, addr);
if (err) {
xenbus_dev_fatal(dev, err, "parsing %s/mac", dev->nodename);
goto out_unlocked;
}
+ eth_hw_addr_set(info->netdev, addr);
info->netback_has_xdp_headroom =
xenbus_read_unsigned(info->xbdev->otherend,
"feature-xdp-headroom", 0);
--
2.31.1
[PATCH net-next 01/12] net: xen: use eth_hw_addr_set()
Commit 406f42fa0d3c ("net-next: When a bond have a massive amount
of VLANs...") introduced a rbtree for faster Ethernet address look
up. To maintain netdev->dev_addr in this tree we need to make all
the writes to it got through appropriate helpers.
Signed-off-by: Jakub Kicinski
---
CC: wei@kernel.org
CC: p...@xen.org
CC: boris.ostrov...@oracle.com
CC: jgr...@suse.com
CC: sstabell...@kernel.org
CC: xen-devel@lists.xenproject.org
---
drivers/net/xen-netback/interface.c | 6 --
drivers/net/xen-netfront.c | 4 +++-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/xen-netback/interface.c
b/drivers/net/xen-netback/interface.c
index c58996c1e230..fe8e21ad8ed9 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -494,6 +494,9 @@ static const struct net_device_ops xenvif_netdev_ops = {
struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
unsigned int handle)
{
+ static const u8 dummy_addr[ETH_ALEN] = {
+ 0xfe, 0xff, 0xff, 0xff, 0xff, 0xff,
+ };
int err;
struct net_device *dev;
struct xenvif *vif;
@@ -551,8 +554,7 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t
domid,
* stolen by an Ethernet bridge for STP purposes.
* (FE:FF:FF:FF:FF:FF)
*/
- eth_broadcast_addr(dev->dev_addr);
- dev->dev_addr[0] &= ~0x01;
+ eth_hw_addr_set(dev, dummy_addr);
netif_carrier_off(dev);
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e31b98403f31..57437e4b8a94 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -2157,6 +2157,7 @@ static int talk_to_netback(struct xenbus_device *dev,
unsigned int max_queues = 0;
struct netfront_queue *queue = NULL;
unsigned int num_queues = 1;
+ u8 addr[ETH_ALEN];
info->netdev->irq = 0;
@@ -2170,11 +2171,12 @@ static int talk_to_netback(struct xenbus_device *dev,
"feature-split-event-channels", 0);
/* Read mac addr. */
- err = xen_net_read_mac(dev, info->netdev->dev_addr);
+ err = xen_net_read_mac(dev, addr);
if (err) {
xenbus_dev_fatal(dev, err, "parsing %s/mac", dev->nodename);
goto out_unlocked;
}
+ eth_hw_addr_set(info->netdev, addr);
info->netback_has_xdp_headroom =
xenbus_read_unsigned(info->xbdev->otherend,
"feature-xdp-headroom", 0);
--
2.31.1
Re: [PATCH] xen/netback: avoid race in xenvif_rx_ring_slots_available()
On Thu, 4 Feb 2021 06:32:32 +0100 Jürgen Groß wrote:
> On 04.02.21 00:48, Jakub Kicinski wrote:
> > On Tue, 2 Feb 2021 08:09:38 +0100 Juergen Gross wrote:
> >> Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
> >> xenvif_rx_ring_slots_available() is no longer called only from the rx
> >> queue kernel thread, so it needs to access the rx queue with the
> >> associated queue held.
> >>
> >> Reported-by: Igor Druzhinin
> >> Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
> >> Cc: sta...@vger.kernel.org
> >> Signed-off-by: Juergen Gross
> >
> > Should we route this change via networking trees? I see the bug did not
> > go through networking :)
>
> I'm fine with either networking or the Xen tree. It should be included
> in 5.11, though. So if you are willing to take it, please do so.
All right, applied to net, it'll most likely hit Linus's tree on Tue.
Thanks!
Re: [PATCH] xen/netback: avoid race in xenvif_rx_ring_slots_available()
On Tue, 2 Feb 2021 08:09:38 +0100 Juergen Gross wrote:
> Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
> xenvif_rx_ring_slots_available() is no longer called only from the rx
> queue kernel thread, so it needs to access the rx queue with the
> associated queue held.
>
> Reported-by: Igor Druzhinin
> Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Juergen Gross
Should we route this change via networking trees? I see the bug did not
go through networking :)
Re: [PATCH v2 0/7] Rid W=1 warnings in Ethernet
On Fri, 15 Jan 2021 13:38:48 + Lee Jones wrote: > Okay, so what would you like me to do? Would you like me to re-submit > the set based only on net-next Yes, rebase your patches on net-next, recheck everything builds okay and resubmit. You should always develop against the tree that will merge your patches. I appreciate for your janitorial work using linux-next is more expedient, but as you can see it causes trouble, this is not the first time your patches don't apply to net-next IIRC.
Re: [PATCH v2 0/7] Rid W=1 warnings in Ethernet
On Thu, 14 Jan 2021 08:33:49 + Lee Jones wrote: > On Wed, 13 Jan 2021, Jakub Kicinski wrote: > > > On Wed, 13 Jan 2021 16:41:16 + Lee Jones wrote: > > > Resending the stragglers again. > > > > > > > > > This set is part of a larger effort attempting to clean-up W=1 > > > > > > kernel builds, which are currently overwhelmingly riddled with > > > > > > niggly little warnings. > > > > > > > > > > > > v2: > > > > > > - Squashed IBM patches > > > > > > - Fixed real issue in SMSC > > > - Added Andrew's Reviewed-by tags on remainder > > > > Does not apply, please rebase on net-next/master. > > These are based on Tuesday's next/master. What's next/master? This is net-next: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/ > I just rebased them now with no issue. > > What conflict are you seeing? Applying: net: ethernet: smsc: smc91x: Fix function name in kernel-doc header error: patch failed: drivers/net/ethernet/smsc/smc91x.c:2192 error: drivers/net/ethernet/smsc/smc91x.c: patch does not apply Patch failed at 0001 net: ethernet: smsc: smc91x: Fix function name in kernel-doc header hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Re: [PATCH v2 0/7] Rid W=1 warnings in Ethernet
On Wed, 13 Jan 2021 16:41:16 + Lee Jones wrote: > Resending the stragglers again. > > > This set is part of a larger effort attempting to clean-up W=1 > > kernel builds, which are currently overwhelmingly riddled with > > niggly little warnings. > > > > v2: > > - Squashed IBM patches > > - Fixed real issue in SMSC > - Added Andrew's Reviewed-by tags on remainder Does not apply, please rebase on net-next/master.
Re: [PATCH 0/8] Rid W=1 warnings in Net
On Thu, 26 Nov 2020 13:38:45 + Lee Jones wrote: > Resending the stragglers. > > This set is part of a larger effort attempting to clean-up W=1 > kernel builds, which are currently overwhelmingly riddled with > niggly little warnings. This set doesn't apply to net-next, please rebase.
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > This series aims to fix almost all remaining fall-through warnings in > > > order to enable -Wimplicit-fallthrough for Clang. > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > > add multiple break/goto/return/fallthrough statements instead of just > > > letting the code fall through to the next case. > > > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > > change[1] is meant to be reverted at some point. So, this patch helps > > > to move in that direction. > > > > > > Something important to mention is that there is currently a discrepancy > > > between GCC and Clang when dealing with switch fall-through to empty case > > > statements or to cases that only contain a break/continue/return > > > statement[2][3][4]. > > > > Are we sure we want to make this change? Was it discussed before? > > > > Are there any bugs Clangs puritanical definition of fallthrough helped > > find? > > > > IMVHO compiler warnings are supposed to warn about issues that could > > be bugs. Falling through to default: break; can hardly be a bug?! > > It's certainly a place where the intent is not always clear. I think > this makes all the cases unambiguous, and doesn't impact the machine > code, since the compiler will happily optimize away any behavioral > redundancy. If none of the 140 patches here fix a real bug, and there is no change to machine code then it sounds to me like a W=2 kind of a warning. I think clang is just being annoying here, but if I'm the only one who feels this way chances are I'm wrong :)
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > This series aims to fix almost all remaining fall-through warnings in > order to enable -Wimplicit-fallthrough for Clang. > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > add multiple break/goto/return/fallthrough statements instead of just > letting the code fall through to the next case. > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > change[1] is meant to be reverted at some point. So, this patch helps > to move in that direction. > > Something important to mention is that there is currently a discrepancy > between GCC and Clang when dealing with switch fall-through to empty case > statements or to cases that only contain a break/continue/return > statement[2][3][4]. Are we sure we want to make this change? Was it discussed before? Are there any bugs Clangs puritanical definition of fallthrough helped find? IMVHO compiler warnings are supposed to warn about issues that could be bugs. Falling through to default: break; can hardly be a bug?!
Re: [Xen-devel] [PATCH net v2] xen-netback: avoid race that can lead to NULL pointer dereference
On Fri, 13 Dec 2019 13:20:40 +, Paul Durrant wrote:
> In function xenvif_disconnect_queue(), the value of queue->rx_irq is
> zeroed *before* queue->task is stopped. Unfortunately that task may call
> notify_remote_via_irq(queue->rx_irq) and calling that function with a
> zero value results in a NULL pointer dereference in evtchn_from_irq().
>
> This patch simply re-orders things, stopping all tasks before zero-ing the
> irq values, thereby avoiding the possibility of the race.
>
> Fixes: 2ac061ce97f4 ("xen/netback: cleanup init and deinit code")
> Signed-off-by: Paul Durrant
> v2:
> - Add 'Fixes' tag and re-work commit comment
I've added Wei's Ack from v1, if the code doesn't change substantially
please keep people's Acks.
Applied, thanks.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/netback: cleanup init and deinit code
On Mon, 21 Oct 2019 07:30:52 +0200, Juergen Gross wrote: > Do some cleanup of the netback init and deinit code: > > - add an omnipotent queue deinit function usable from > xenvif_disconnect_data() and the error path of xenvif_connect_data() > - only install the irq handlers after initializing all relevant items > (especially the kthreads related to the queue) > - there is no need to use get_task_struct() after creating a kthread > and using put_task_struct() again after having stopped it. > - use kthread_run() instead of kthread_create() to spare the call of > wake_up_process(). > > Signed-off-by: Juergen Gross > Reviewed-by: Paul Durrant Applied to net-next, thanks! ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
