[PATCH 5.4] xen-netback: move removal of "hotplug-status" to the right place
The removal of "hotplug-status" has moved around a bit. First it was
moved from netback_remove() to hotplug_status_changed() in upstream
commit 1f2565780e9b ("xen-netback: remove 'hotplug-status' once it has
served its purpose"). Then the change was reverted in upstream commit
0f4558ae9187 ("Revert "xen-netback: remove 'hotplug-status' once it has
served its purpose""), but it moved the removal to backend_disconnect().
Then the upstream commit c55f34b6aec2 ("xen-netback: only remove
'hotplug-status' when the vif is actually destroyed") moved it finally
back to netback_remove(). The thing to note being it is removed
unconditionally this time around.
The story on v5.4.y adds to this confusion. Commit 60e4e3198ce8 ("Revert
"xen-netback: remove 'hotplug-status' once it has served its purpose"")
is backported to v5.4.y but the original commit that it tries to revert
was never present on 5.4. So the backport incorrectly ends up just
adding another xenbus_rm() of "hotplug-status" in backend_disconnect().
Now in v5.4.y it is removed in both backend_disconnect() and
netback_remove(). But it should only be removed in netback_remove(), as
the upstream version does.
Removing "hotplug-status" in backend_disconnect() causes problems when
the frontend unilaterally disconnects, as explained in
c55f34b6aec2 ("xen-netback: only remove 'hotplug-status' when the vif is
actually destroyed").
Remove "hotplug-status" in the same place as it is done on the upstream
version to ensure unilateral re-connection of frontend continues to
work.
Fixes: 60e4e3198ce8 ("Revert "xen-netback: remove 'hotplug-status' once it has
served its purpose"")
Signed-off-by: Pratyush Yadav
---
drivers/net/xen-netback/xenbus.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 44e353dd2ba1..43bd881ab3dd 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -202,10 +202,10 @@ static int netback_remove(struct xenbus_device *dev)
set_backend_state(be, XenbusStateClosed);
unregister_hotplug_status_watch(be);
+ xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
if (be->vif) {
kobject_uevent(>dev.kobj, KOBJ_OFFLINE);
xen_unregister_watchers(be->vif);
- xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
xenvif_free(be->vif);
be->vif = NULL;
}
@@ -435,7 +435,6 @@ static void backend_disconnect(struct backend_info *be)
unsigned int queue_index;
xen_unregister_watchers(vif);
- xenbus_rm(XBT_NIL, be->dev->nodename, "hotplug-status");
#ifdef CONFIG_DEBUG_FS
xenvif_debugfs_delif(vif);
#endif /* CONFIG_DEBUG_FS */
--
2.38.1
Re: Xen Security Advisory 424 v1 (CVE-2022-42328,CVE-2022-42329) - Guests can trigger deadlock in Linux netback driver
Hi,
I noticed one interesting thing about this patch but I'm not familiar
enough with the driver to say for sure what the right thing is.
On Tue, Dec 06 2022, Xen.org security team wrote:
[...]
>
> From cfdf8fd81845734b6152b4617746c1127ec52228 Mon Sep 17 00:00:00 2001
> From: Juergen Gross
> Date: Tue, 6 Dec 2022 08:54:24 +0100
> Subject: [PATCH] xen/netback: don't call kfree_skb() with interrupts disabled
>
> It is not allowed to call kfree_skb() from hardware interrupt
> context or with interrupts being disabled. So remove kfree_skb()
> from the spin_lock_irqsave() section and use the already existing
> "drop" label in xenvif_start_xmit() for dropping the SKB. At the
> same time replace the dev_kfree_skb() call there with a call of
> dev_kfree_skb_any(), as xenvif_start_xmit() can be called with
> disabled interrupts.
>
> This is XSA-424 / CVE-2022-42328 / CVE-2022-42329.
>
> Fixes: be81992f9086 ("xen/netback: don't queue unlimited number of packages")
> Reported-by: Yang Yingliang
> Signed-off-by: Juergen Gross
> Reviewed-by: Jan Beulich
> ---
> drivers/net/xen-netback/common.h| 2 +-
> drivers/net/xen-netback/interface.c | 6 --
> drivers/net/xen-netback/rx.c| 8 +---
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/xen-netback/common.h
> b/drivers/net/xen-netback/common.h
> index 1545cbee77a4..3dbfc8a6924e 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -386,7 +386,7 @@ int xenvif_dealloc_kthread(void *data);
> irqreturn_t xenvif_ctrl_irq_fn(int irq, void *data);
>
> bool xenvif_have_rx_work(struct xenvif_queue *queue, bool test_kthread);
> -void xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb);
> +bool xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb);
>
> void xenvif_carrier_on(struct xenvif *vif);
>
> diff --git a/drivers/net/xen-netback/interface.c
> b/drivers/net/xen-netback/interface.c
> index 650fa180220f..f3f2c07423a6 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -254,14 +254,16 @@ xenvif_start_xmit(struct sk_buff *skb, struct
> net_device *dev)
> if (vif->hash.alg == XEN_NETIF_CTRL_HASH_ALGORITHM_NONE)
> skb_clear_hash(skb);
>
> - xenvif_rx_queue_tail(queue, skb);
> + if (!xenvif_rx_queue_tail(queue, skb))
> + goto drop;
> +
> xenvif_kick_thread(queue);
>
> return NETDEV_TX_OK;
>
> drop:
> vif->dev->stats.tx_dropped++;
Now tx_dropped is incremented on packet drop...
> - dev_kfree_skb(skb);
> + dev_kfree_skb_any(skb);
> return NETDEV_TX_OK;
> }
>
> diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
> index 932762177110..0ba754ebc5ba 100644
> --- a/drivers/net/xen-netback/rx.c
> +++ b/drivers/net/xen-netback/rx.c
> @@ -82,9 +82,10 @@ static bool xenvif_rx_ring_slots_available(struct
> xenvif_queue *queue)
> return false;
> }
>
> -void xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb)
> +bool xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb)
> {
> unsigned long flags;
> + bool ret = true;
>
> spin_lock_irqsave(>rx_queue.lock, flags);
>
> @@ -92,8 +93,7 @@ void xenvif_rx_queue_tail(struct xenvif_queue *queue,
> struct sk_buff *skb)
> struct net_device *dev = queue->vif->dev;
>
> netif_tx_stop_queue(netdev_get_tx_queue(dev, queue->id));
> - kfree_skb(skb);
> - queue->vif->dev->stats.rx_dropped++;
... but earlier rx_dropped was incremented.
Which one is actually correct? This line was added by be81992f9086b
("xen/netback: don't queue unlimited number of packages"), which was the
fix for XSA-392. I think incrementing tx_dropped is the right thing to
do, as was done before XSA-392 but it would be nice if someone else
takes a look at this as well.
> + ret = false;
> } else {
> if (skb_queue_empty(>rx_queue))
> xenvif_update_needed_slots(queue, skb);
> @@ -104,6 +104,8 @@ void xenvif_rx_queue_tail(struct xenvif_queue *queue,
> struct sk_buff *skb)
> }
>
> spin_unlock_irqrestore(>rx_queue.lock, flags);
> +
> + return ret;
> }
>
> static struct sk_buff *xenvif_rx_dequeue(struct xenvif_queue *queue)
--
Regards,
Pratyush Yadav
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested
On 02/09/22 01:08PM, Juergen Gross wrote:
> On 02.09.22 11:53, Pratyush Yadav wrote:
> > On 31/08/22 04:58PM, SeongJae Park wrote:
> > > The advertisement of the persistent grants feature (writing
> > > 'feature-persistent' to xenbus) should mean not the decision for using
> > > the feature but only the availability of the feature. However, commit
> > > aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent
> > > grants") made a field of blkback, which was a place for saving only the
> > > negotiation result, to be used for yet another purpose: caching of the
> > > 'feature_persistent' parameter value. As a result, the advertisement,
> > > which should follow only the parameter value, becomes inconsistent.
> > >
> > > This commit fixes the misuse of the semantic by making blkback saves the
> > > parameter value in a separate place and advertises the support based on
> > > only the saved value.
> > >
> > > Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of
> > > persistent grants")
> > > Cc: # 5.10.x
> > > Suggested-by: Juergen Gross
> > > Signed-off-by: SeongJae Park
> > > ---
> > > drivers/block/xen-blkback/common.h | 3 +++
> > > drivers/block/xen-blkback/xenbus.c | 6 --
> > > 2 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/block/xen-blkback/common.h
> > > b/drivers/block/xen-blkback/common.h
> > > index bda5c815e441..a28473470e66 100644
> > > --- a/drivers/block/xen-blkback/common.h
> > > +++ b/drivers/block/xen-blkback/common.h
> > > @@ -226,6 +226,9 @@ struct xen_vbd {
> > > sector_tsize;
> > > unsigned intflush_support:1;
> > > unsigned intdiscard_secure:1;
> > > + /* Connect-time cached feature_persistent parameter value */
> > > + unsigned intfeature_gnt_persistent_parm:1;
> >
> > Continuing over from the previous version:
> >
> > > > If feature_gnt_persistent_parm is always going to be equal to
> > > > feature_persistent, then why introduce it at all? Why not just use
> > > > feature_persistent directly? This way you avoid adding an extra flag
> > > > whose purpose is not immediately clear, and you also avoid all the
> > > > mess with setting this flag at the right time.
> > >
> > > Mainly because the parameter should read twice (once for
> > > advertisement, and once later just before the negotitation, for
> > > checking if we advertised or not), and the user might change the
> > > parameter value between the two reads.
> > >
> > > For the detailed available sequence of the race, you could refer to the
> > > prior conversation[1].
> > >
> > > [1]
> > > https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/
> >
> > Okay, I see. Thanks for the pointer. But still, I think it would be
> > better to not maintain two copies of the value. How about doing:
> >
> > blkif->vbd.feature_gnt_persistent =
> > xenbus_read_unsigned(dev->nodename, "feature-persistent", 0) &&
> > xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
> >
> > This makes it quite clear that we only enable persistent grants if
> > _both_ ends support it. We can do the same for blkfront.
>
> I prefer it as is, as it will not rely on nobody having modified the
> Xenstore node (which would in theory be possible).
Okay. In that case,
Reviewed-by: Pratyush Yadav
--
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested
On 31/08/22 04:58PM, SeongJae Park wrote:
> The advertisement of the persistent grants feature (writing
> 'feature-persistent' to xenbus) should mean not the decision for using
> the feature but only the availability of the feature. However, commit
> aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent
> grants") made a field of blkback, which was a place for saving only the
> negotiation result, to be used for yet another purpose: caching of the
> 'feature_persistent' parameter value. As a result, the advertisement,
> which should follow only the parameter value, becomes inconsistent.
>
> This commit fixes the misuse of the semantic by making blkback saves the
> parameter value in a separate place and advertises the support based on
> only the saved value.
>
> Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of
> persistent grants")
> Cc: # 5.10.x
> Suggested-by: Juergen Gross
> Signed-off-by: SeongJae Park
> ---
> drivers/block/xen-blkback/common.h | 3 +++
> drivers/block/xen-blkback/xenbus.c | 6 --
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/common.h
> b/drivers/block/xen-blkback/common.h
> index bda5c815e441..a28473470e66 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -226,6 +226,9 @@ struct xen_vbd {
> sector_tsize;
> unsigned intflush_support:1;
> unsigned intdiscard_secure:1;
> + /* Connect-time cached feature_persistent parameter value */
> + unsigned intfeature_gnt_persistent_parm:1;
Continuing over from the previous version:
> > If feature_gnt_persistent_parm is always going to be equal to
> > feature_persistent, then why introduce it at all? Why not just use
> > feature_persistent directly? This way you avoid adding an extra flag
> > whose purpose is not immediately clear, and you also avoid all the
> > mess with setting this flag at the right time.
>
> Mainly because the parameter should read twice (once for
> advertisement, and once later just before the negotitation, for
> checking if we advertised or not), and the user might change the
> parameter value between the two reads.
>
> For the detailed available sequence of the race, you could refer to the
> prior conversation[1].
>
> [1] https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/
Okay, I see. Thanks for the pointer. But still, I think it would be
better to not maintain two copies of the value. How about doing:
blkif->vbd.feature_gnt_persistent =
xenbus_read_unsigned(dev->nodename, "feature-persistent", 0) &&
xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
This makes it quite clear that we only enable persistent grants if
_both_ ends support it. We can do the same for blkfront.
> + /* Persistent grants feature negotiation result */
> unsigned intfeature_gnt_persistent:1;
> unsigned intoverflow_max_grants:1;
> };
> diff --git a/drivers/block/xen-blkback/xenbus.c
> b/drivers/block/xen-blkback/xenbus.c
> index ee7ad2fb432d..c0227dfa4688 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -907,7 +907,7 @@ static void connect(struct backend_info *be)
> xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
>
> err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> - be->blkif->vbd.feature_gnt_persistent);
> + be->blkif->vbd.feature_gnt_persistent_parm);
> if (err) {
> xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
> dev->nodename);
> @@ -1085,7 +1085,9 @@ static int connect_ring(struct backend_info *be)
> return -ENOSYS;
> }
>
> - blkif->vbd.feature_gnt_persistent = feature_persistent &&
> + blkif->vbd.feature_gnt_persistent_parm = feature_persistent;
> + blkif->vbd.feature_gnt_persistent =
> + blkif->vbd.feature_gnt_persistent_parm &&
> xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
>
> blkif->vbd.overflow_max_grants = 0;
> --
> 2.25.1
>
--
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Re: [PATCH 1/2] xen-blkback: Advertise feature-persistent as user requested
Hi,
On 25/08/22 04:15PM, SeongJae Park wrote:
> Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> when connect") made blkback to advertise its support of the persistent
> grants feature only if the user sets the 'feature_persistent' parameter
> of the driver and the frontend advertised its support of the feature.
> However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> 'feature_persistent' parameter when connect") made the blkfront to work
> in the same way. That is, blkfront also advertises its support of the
> persistent grants feature only if the user sets the 'feature_persistent'
> parameter of the driver and the backend advertised its support of the
> feature.
>
> Hence blkback and blkfront will never advertise their support of the
> feature but wait until the other advertises the support, even though
> users set the 'feature_persistent' parameters of the drivers. As a
> result, the persistent grants feature is disabled always regardless of
> the 'feature_persistent' values[1].
>
> The problem comes from the misuse of the semantic of the advertisement
> of the feature. The advertisement of the feature should means only
> availability of the feature not the decision for using the feature.
> However, current behavior is working in the wrong way.
>
> This commit fixes the issue by making the blkback advertises its support
> of the feature as user requested via 'feature_persistent' parameter
> regardless of the otherend's support of the feature.
>
> [1]
> https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f...@suse.com/
>
> Fixes: e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter when
> connect")
> Cc: # 5.10.x
> Reported-by: Marek Marczykowski-Górecki
> Suggested-by: Juergen Gross
> Signed-off-by: SeongJae Park
> ---
> drivers/block/xen-blkback/common.h | 3 +++
> drivers/block/xen-blkback/xenbus.c | 6 --
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/common.h
> b/drivers/block/xen-blkback/common.h
> index bda5c815e441..a28473470e66 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -226,6 +226,9 @@ struct xen_vbd {
> sector_tsize;
> unsigned intflush_support:1;
> unsigned intdiscard_secure:1;
> + /* Connect-time cached feature_persistent parameter value */
> + unsigned intfeature_gnt_persistent_parm:1;
> + /* Persistent grants feature negotiation result */
> unsigned intfeature_gnt_persistent:1;
> unsigned intoverflow_max_grants:1;
> };
> diff --git a/drivers/block/xen-blkback/xenbus.c
> b/drivers/block/xen-blkback/xenbus.c
> index ee7ad2fb432d..c0227dfa4688 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -907,7 +907,7 @@ static void connect(struct backend_info *be)
> xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
>
> err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> - be->blkif->vbd.feature_gnt_persistent);
> + be->blkif->vbd.feature_gnt_persistent_parm);
> if (err) {
> xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
>dev->nodename);
> @@ -1085,7 +1085,9 @@ static int connect_ring(struct backend_info *be)
> return -ENOSYS;
> }
>
> - blkif->vbd.feature_gnt_persistent = feature_persistent &&
> + blkif->vbd.feature_gnt_persistent_parm = feature_persistent;
If feature_gnt_persistent_parm is always going to be equal to
feature_persistent, then why introduce it at all? Why not just use
feature_persistent directly? This way you avoid adding an extra flag
whose purpose is not immediately clear, and you also avoid all the mess
with setting this flag at the right time.
> + blkif->vbd.feature_gnt_persistent =
> + blkif->vbd.feature_gnt_persistent_parm &&
> xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
>
> blkif->vbd.overflow_max_grants = 0;
> --
> 2.25.1
>
>
Re: [PATCH 2/2] xen-blkfront: Advertise feature-persistent as user requested
On 25/08/22 04:15PM, SeongJae Park wrote:
> Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> when connect") made blkback to advertise its support of the persistent
> grants feature only if the user sets the 'feature_persistent' parameter
> of the driver and the frontend advertised its support of the feature.
> However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> 'feature_persistent' parameter when connect") made the blkfront to work
> in the same way. That is, blkfront also advertises its support of the
> persistent grants feature only if the user sets the 'feature_persistent'
> parameter of the driver and the backend advertised its support of the
> feature.
>
> Hence blkback and blkfront will never advertise their support of the
> feature but wait until the other advertises the support, even though
> users set the 'feature_persistent' parameters of the drivers. As a
> result, the persistent grants feature is disabled always regardless of
> the 'feature_persistent' values[1].
>
> The problem comes from the misuse of the semantic of the advertisement
> of the feature. The advertisement of the feature should means only
> availability of the feature not the decision for using the feature.
> However, current behavior is working in the wrong way.
>
> This commit fixes the issue by making the blkfront advertises its
> support of the feature as user requested via 'feature_persistent'
> parameter regardless of the otherend's support of the feature.
>
> [1]
> https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f...@suse.com/
>
> Fixes: 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when
> connect")
> Cc: # 5.10.x
> Reported-by: Marek Marczykowski-Górecki
> Suggested-by: Juergen Gross
> Signed-off-by: SeongJae Park
> ---
> drivers/block/xen-blkfront.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 8e56e69fb4c4..dfae08115450 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -213,6 +213,9 @@ struct blkfront_info
> unsigned int feature_fua:1;
> unsigned int feature_discard:1;
> unsigned int feature_secdiscard:1;
> + /* Connect-time cached feature_persistent parameter */
> + unsigned int feature_persistent_parm:1;
> + /* Persistent grants feature negotiation result */
> unsigned int feature_persistent:1;
> unsigned int bounce:1;
> unsigned int discard_granularity;
> @@ -1848,7 +1851,7 @@ static int talk_to_blkback(struct xenbus_device *dev,
> goto abort_transaction;
> }
> err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> - info->feature_persistent);
> + info->feature_persistent_parm);
> if (err)
> dev_warn(>dev,
>"writing persistent grants feature to xenbus");
> @@ -2281,7 +2284,8 @@ static void blkfront_gather_backend_features(struct
> blkfront_info *info)
> if (xenbus_read_unsigned(info->xbdev->otherend, "feature-discard", 0))
> blkfront_setup_discard(info);
>
> - if (feature_persistent)
> + info->feature_persistent_parm = feature_persistent;
Same question as before. Why not just use feature_persistent directly?
> + if (info->feature_persistent_parm)
> info->feature_persistent =
> !!xenbus_read_unsigned(info->xbdev->otherend,
> "feature-persistent", 0);
Aside: IMO this would look nicer as below:
info->feature_persistent = feature_persistent &&
!!xenbus_read_unsigned();
