Re: [PATCH v2] xen-netfront: fix potential deadlock in xennet_remove()

[David Miller]

From: Andrea Righi 
Date: Fri, 24 Jul 2020 10:59:10 +0200

> There's a potential race in xennet_remove(); this is what the driver is
> doing upon unregistering a network device:
> 
>   1. state = read bus state
>   2. if state is not "Closed":
>   3.request to set state to "Closing"
>   4.wait for state to be set to "Closing"
>   5.request to set state to "Closed"
>   6.wait for state to be set to "Closed"
> 
> If the state changes to "Closed" immediately after step 1 we are stuck
> forever in step 4, because the state will never go back from "Closed" to
> "Closing".
> 
> Make sure to check also for state == "Closed" in step 4 to prevent the
> deadlock.
> 
> Also add a 5 sec timeout any time we wait for the bus state to change,
> to avoid getting stuck forever in wait_event().
> 
> Signed-off-by: Andrea Righi 
> ---
> Changes in v2:
>  - remove all dev_dbg() calls (as suggested by David Miller)

Applied, thank you.

Re: [PATCH] xen-netfront: fix potential deadlock in xennet_remove()

[David Miller]

From: Andrea Righi 
Date: Wed, 22 Jul 2020 08:52:11 +0200

> +static int xennet_remove(struct xenbus_device *dev)
> +{
> + struct netfront_info *info = dev_get_drvdata(>dev);
> +
> + dev_dbg(>dev, "%s\n", dev->nodename);

These kinds of debugging messages provide zero context and are so much
less useful than simply using tracepoints which are more universally
available than printk debugging facilities.

Please remove all of the dev_dbg() calls from this patch.

Re: [PATCH][next] xen-netfront: remove redundant assignment to variable 'act'

[David Miller]

From: Colin King 
Date: Thu,  2 Jul 2020 15:22:23 +0100

> From: Colin Ian King 
> 
> The variable act is being initialized with a value that is
> never read and it is being updated later with a new value. The
> initialization is redundant and can be removed.
> 
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King 

Applied, thank you.

Re: [Xen-devel] [PATCH v3 4/4] xen/netback: fix grant copy across page boundary

[David Miller]

From: Sergey Dyasli 
Date: Fri, 7 Feb 2020 14:26:52 +

> From: Ross Lagerwall 
> 
> When KASAN (or SLUB_DEBUG) is turned on, there is a higher chance that
> non-power-of-two allocations are not aligned to the next power of 2 of
> the size. Therefore, handle grant copies that cross page boundaries.
> 
> Signed-off-by: Ross Lagerwall 
> Signed-off-by: Sergey Dyasli 
> Acked-by: Paul Durrant 

This is part of a larger patch series to which netdev was not CC:'d

Where is this patch targetted to be applied?

Do you expect a networking ACK on this?

Please do not submit patches in such an ambiguous manner like this
in the future, thank you.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] net: xen-netback: hash.c: Use built-in RCU list checking

[David Miller]

From: madhuparnabhowmi...@gmail.com
Date: Wed, 15 Jan 2020 21:25:53 +0530

> From: Madhuparna Bhowmik 
> 
> list_for_each_entry_rcu has built-in RCU and lock checking.
> Pass cond argument to list_for_each_entry_rcu.
> 
> Signed-off-by: Madhuparna Bhowmik 

Applied to net-next.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next] xen-netback: support dynamic unbind/bind

[David Miller]

From: Paul Durrant 
Date: Mon, 23 Dec 2019 09:59:23 +

> By re-attaching RX, TX, and CTL rings during connect() rather than
> assuming they are freshly allocated (i.e. assuming the counters are zero),
> and avoiding forcing state to Closed in netback_remove() it is possible
> for vif instances to be unbound and re-bound from and to (respectively) a
> running guest.
> 
> Dynamic unbind/bind is a highly useful feature for a backend module as it
> allows it to be unloaded and re-loaded (i.e. updated) without requiring
> domUs to be halted.
> 
> This has been tested by running iperf as a server in the test VM and
> then running a client against it in a continuous loop, whilst also
> running:
> 
> while true;
>   do echo vif-$DOMID-$VIF >unbind;
>   echo down;
>   rmmod xen-netback;
>   echo unloaded;
>   modprobe xen-netback;
>   cd $(pwd);
>   brctl addif xenbr0 vif$DOMID.$VIF;
>   ip link set vif$DOMID.$VIF up;
>   echo up;
>   sleep 5;
>   done
> 
> in dom0 from /sys/bus/xen-backend/drivers/vif to continuously unbind,
> unload, re-load, re-bind and re-plumb the backend.
> 
> Clearly a performance drop was seen but no TCP connection resets were
> observed during this test and moreover a parallel SSH connection into the
> guest remained perfectly usable throughout.
> 
> Signed-off-by: Paul Durrant 

Applied, thank you.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next 3/3] xen-netback: remove 'hotplug-status' once it has served its purpose

[David Miller]

From: Paul Durrant 
Date: Tue, 17 Dec 2019 13:32:18 +

> Removing the 'hotplug-status' node in netback_remove() is wrong; the script
> may not have completed. Only remove the node once the watch has fired and
> has been unregistered.
> 
> Signed-off-by: Paul Durrant 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next 2/3] xen-netback: switch state to InitWait at the end of netback_probe()...

[David Miller]

From: Paul Durrant 
Date: Tue, 17 Dec 2019 13:32:17 +

> ...as the comment above the function states.
> 
> The switch to Initialising at the start of the function is somewhat bogus
> as the toolstack will have set that initial state anyway. To behave
> correctly, a backend should switch to InitWait once it has set up all
> xenstore values that may be required by a initialising frontend. This
> patch calls backend_switch_state() to make the transition at the
> appropriate point.
> 
> NOTE: backend_switch_state() ignores errors from xenbus_switch_state()
>   and so this patch removes an error path from netback_probe(). This
>   means a failure to change state at this stage (in the absence of
>   other failures) will leave the device instantiated. This is highly
>   unlikley to happen as a failure to change state would indicate a
>   failure to write to xenstore, and that will trigger other error
>   paths. Also, a 'stuck' device can still be cleaned up using 'unbind'
>   in any case.
> 
> Signed-off-by: Paul Durrant 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next 1/3] xen-netback: move netback_probe() and netback_remove() to the end...

[David Miller]

From: Paul Durrant 
Date: Tue, 17 Dec 2019 13:32:16 +

> ...of xenbus.c
> 
> This is a cosmetic function re-ordering to reduce churn in a subsequent
> patch. Some style fix-up was done to make checkpatch.pl happier.
> 
> No functional change.
> 
> Signed-off-by: Paul Durrant 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next] xen-netback: get rid of old udev related code

[David Miller]

From: Paul Durrant 
Date: Thu, 12 Dec 2019 13:54:06 +

> In the past it used to be the case that the Xen toolstack relied upon
> udev to execute backend hotplug scripts. However this has not been the
> case for many releases now and removal of the associated code in
> xen-netback shortens the source by more than 100 lines, and removes much
> complexity in the interaction with the xenstore backend state.
> 
> NOTE: xen-netback is the only xenbus driver to have a functional uevent()
>   method. The only other driver to have a method at all is
>   pvcalls-back, and currently pvcalls_back_uevent() simply returns 0.
>   Hence this patch also facilitates further cleanup.
> 
> Signed-off-by: Paul Durrant 

If userspace ever used this stuff, I seriously doubt you can remove this
even if it hasn't been used in 5+ years.

Sorry.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net] xen-netback: avoid race that can lead to NULL pointer dereference

[David Miller]

From: Paul Durrant 
Date: Thu, 12 Dec 2019 12:37:23 +

> Commit 2ac061ce97f4 ("xen/netback: cleanup init and deinit code")
> introduced a problem. 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.
> 
> Signed-off-by: Paul Durrant 

Please repost this with an appropriate Fixes: tag.

And then you can removed the explicit commit reference from the log message
and simply say "The commit mentioned in the Fixes tag introduced a problen ..."

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/netback: fix error path of xenvif_connect_data()

[David Miller]

From: Juergen Gross 
Date: Fri, 18 Oct 2019 09:45:49 +0200

> xenvif_connect_data() calls module_put() in case of error. This is
> wrong as there is no related module_get().
> 
> Remove the superfluous module_put().
> 
> Fixes: 279f438e36c0a7 ("xen-netback: Don't destroy the netdev until the vif 
> is shut down")
> Signed-off-by: Juergen Gross 
> Reviewed-by: Paul Durrant 

Applied and queued up for -stable.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/2] xen/netback: bug fix and cleanup

[David Miller]

From: Juergen Gross 
Date: Mon, 14 Oct 2019 11:09:08 +0200

> One bugfix (patch 1) I stumbled over while doing a cleanup (patch 2)
> of the xen-netback init/deinit code.

Please do not mix cleanups and genuine bug fixes.

Submit the bug fix targetting the 'net' GIT tree, and once that eventually
gets merged into 'net-next' you can submit the cleanup against that.

Thanks.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/1] xen-netfront: do not use ~0U as error return value for xennet_fill_frags()

[David Miller]

From: Dongli Zhang 
Date: Tue,  1 Oct 2019 21:56:41 +0800

> xennet_fill_frags() uses ~0U as return value when the sk_buff is not able
> to cache extra fragments. This is incorrect because the return type of
> xennet_fill_frags() is RING_IDX and 0x is an expected value for
> ring buffer index.
> 
> In the situation when the rsp_cons is approaching 0x, the return
> value of xennet_fill_frags() may become 0x which xennet_poll() (the
> caller) would regard as error. As a result, queue->rx.rsp_cons is set
> incorrectly because it is updated only when there is error. If there is no
> error, xennet_poll() would be responsible to update queue->rx.rsp_cons.
> Finally, queue->rx.rsp_cons would point to the rx ring buffer entries whose
> queue->rx_skbs[i] and queue->grant_rx_ref[i] are already cleared to NULL.
> This leads to NULL pointer access in the next iteration to process rx ring
> buffer entries.
> 
> The symptom is similar to the one fixed in
> commit 00b368502d18 ("xen-netfront: do not assume sk_buff_head list is
> empty in error handling").
> 
> This patch changes the return type of xennet_fill_frags() to indicate
> whether it is successful or failed. The queue->rx.rsp_cons will be
> always updated inside this function.
> 
> Fixes: ad4f15dc2c70 ("xen/netfront: don't bug in case of too many frags")
> Signed-off-by: Dongli Zhang 

Applied and queued up for -stable.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/1] xen-netfront: do not assume sk_buff_head list is empty in error handling

[David Miller]

From: Dongli Zhang 
Date: Mon, 16 Sep 2019 11:46:59 +0800

> When skb_shinfo(skb) is not able to cache extra fragment (that is,
> skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS), xennet_fill_frags() assumes
> the sk_buff_head list is already empty. As a result, cons is increased only
> by 1 and returns to error handling path in xennet_poll().
> 
> However, if the sk_buff_head list is not empty, queue->rx.rsp_cons may be
> set incorrectly. That is, queue->rx.rsp_cons would point to the rx ring
> buffer entries whose queue->rx_skbs[i] and queue->grant_rx_ref[i] are
> already cleared to NULL. This leads to NULL pointer access in the next
> iteration to process rx ring buffer entries.
> 
> Below is how xennet_poll() does error handling. All remaining entries in
> tmpq are accounted to queue->rx.rsp_cons without assuming how many
> outstanding skbs are remained in the list.
> 
>  985 static int xennet_poll(struct napi_struct *napi, int budget)
> ... ...
> 1032   if (unlikely(xennet_set_skb_gso(skb, gso))) {
> 1033   __skb_queue_head(, skb);
> 1034   queue->rx.rsp_cons += skb_queue_len();
> 1035   goto err;
> 1036   }
> 
> It is better to always have the error handling in the same way.
> 
> Fixes: ad4f15dc2c70 ("xen/netfront: don't bug in case of too many frags")
> Signed-off-by: Dongli Zhang 

Applied and queued up for -stable.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next] MAINTAINERS: xen-netback: update my email address

[David Miller]

From: Paul Durrant 
Date: Fri, 13 Sep 2019 13:47:27 +0100

> My Citrix email address will expire shortly.
> 
> Signed-off-by: Paul Durrant 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen-netback: no need to check return value of debugfs_create functions

[David Miller]

From: Greg Kroah-Hartman 
Date: Sat, 10 Aug 2019 12:31:08 +0200

> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 
> Cc: Wei Liu 
> Cc: Paul Durrant 
> Cc: xen-devel@lists.xenproject.org
> Cc: net...@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman 

Applied to net-next.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/netback: Reset nr_frags before freeing skb

[David Miller]

From: Ross Lagerwall 
Date: Mon, 5 Aug 2019 16:34:34 +0100

> At this point nr_frags has been incremented but the frag does not yet
> have a page assigned so freeing the skb results in a crash. Reset
> nr_frags before freeing the skb to prevent this.
> 
> Signed-off-by: Ross Lagerwall 

Applied and queued up for -stable.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen-netback: remove redundant assignment to err

[David Miller]

From: Colin King 
Date: Thu, 30 May 2019 20:04:38 +0100

> From: Colin Ian King 
> 
> The variable err is assigned with the value -ENOMEM that is never
> read and it is re-assigned a new value later on.  The assignment is
> redundant and can be removed.
> 
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King 

Applied to net-next.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next] xen-netfront: mark expected switch fall-through

[David Miller]

From: "Gustavo A. R. Silva" 
Date: Mon, 15 Apr 2019 16:11:41 -0500

> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
> 
> This patch fixes the following warning:
> 
> drivers/net/xen-netfront.c: In function ‘netback_changed’:
> drivers/net/xen-netfront.c:2038:6: warning: this statement may fall through 
> [-Wimplicit-fallthrough=]
>if (dev->state == XenbusStateClosed)
>   ^
> drivers/net/xen-netfront.c:2041:2: note: here
>   case XenbusStateClosing:
>   ^~~~
> 
> Warning level 3 was used: -Wimplicit-fallthrough=3
> 
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
> 
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
> 
> Signed-off-by: Gustavo A. R. Silva 

Applied.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/1] xen-netback: add reference from xenvif to backend_info to facilitate coredump analysis

[David Miller]

From: Dongli Zhang 
Date: Fri, 12 Apr 2019 14:53:24 +0800

> During coredump analysis, it is not easy to obtain the address of
> backend_info in xen-netback.
> 
> So far there are two ways to obtain backend_info:
> 
> 1. Do what xenbus_device_find() does for vmcore to find the xenbus_device
> and then derive it from dev_get_drvdata().
> 
> 2. Extract backend_info from callstack of xenwatch (e.g., netback_remove()
> or frontend_changed()).
> 
> This patch adds a reference from xenvif to backend_info so that it would be
> much more easier to obtain backend_info during coredump analysis.
> 
> Signed-off-by: Dongli Zhang 

Applied to net-next.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen-netback: don't populate the hash cache on XenBus disconnect

[David Miller]

From: Igor Druzhinin 
Date: Thu, 28 Feb 2019 14:11:26 +

> Occasionally, during the disconnection procedure on XenBus which
> includes hash cache deinitialization there might be some packets
> still in-flight on other processors. Handling of these packets includes
> hashing and hash cache population that finally results in hash cache
> data structure corruption.
> 
> In order to avoid this we prevent hashing of those packets if there
> are no queues initialized. In that case RCU protection of queues guards
> the hash cache as well.
> 
> Signed-off-by: Igor Druzhinin 

Applied and queued up for -stable, thanks.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] xen-netback: fix occasional leak of grant ref mappings under memory pressure

[David Miller]

From: Igor Druzhinin 
Date: Thu, 28 Feb 2019 12:48:03 +

> Zero-copy callback flag is not yet set on frag list skb at the moment
> xenvif_handle_frag_list() returns -ENOMEM. This eventually results in
> leaking grant ref mappings since xenvif_zerocopy_callback() is never
> called for these fragments. Those eventually build up and cause Xen
> to kill Dom0 as the slots get reused for new mappings:
> 
> "d0v0 Attempt to implicitly unmap a granted PTE c01329fce005"
> 
> That behavior is observed under certain workloads where sudden spikes
> of page cache writes coexist with active atomic skb allocations from
> network traffic. Additionally, rework the logic to deal with frag_list
> deallocation in a single place.
> 
> Signed-off-by: Paul Durrant 
> Signed-off-by: Igor Druzhinin 

Applied and queued up for -stable, thanks.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next] xen-netback: mark expected switch fall-through

[David Miller]

From: "Gustavo A. R. Silva" 
Date: Fri, 8 Feb 2019 13:58:38 -0600

> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
> 
> Warning level 3 was used: -Wimplicit-fallthrough=3
> 
> This patch is part of the ongoing efforts to enabling
> -Wimplicit-fallthrough.
> 
> Signed-off-by: Gustavo A. R. Silva 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 15/21] sparc: add checks for the return value of memblock_alloc*()

[David Miller]

From: Mike Rapoport 
Date: Wed, 16 Jan 2019 15:44:15 +0200

> Add panic() calls if memblock_alloc*() returns NULL.
> 
> Most of the changes are simply addition of
> 
> if(!ptr)
> panic();
> 
> statements after the calls to memblock_alloc*() variants.
> 
> Exceptions are pcpu_populate_pte() and kernel_map_range() that were
> slightly refactored to accommodate the change.
> 
> Signed-off-by: Mike Rapoport 

Acked-by: David S. Miller 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/netfront: tolerate frags with no data

[David Miller]

From: Juergen Gross 
Date: Tue, 18 Dec 2018 16:06:19 +0100

> At least old Xen net backends seem to send frags with no real data
> sometimes. In case such a fragment happens to occur with the frag limit
> already reached the frontend will BUG currently even if this situation
> is easily recoverable.
> 
> Modify the BUG_ON() condition accordingly.
> 
> Tested-by: Dietmar Hahn 
> Signed-off-by: Juergen Gross 

Applied and queued up for -stable.

But many of these BUG's in the driver should be converted to
WARNs and recovery code added.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] net: xenbus: convert to DEFINE_SHOW_ATTRIBUTE

[David Miller]

From: Yangtao Li 
Date: Mon, 10 Dec 2018 10:53:29 -0500

> Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
> 
> Signed-off-by: Yangtao Li 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] remove the ->mapping_error method from dma_map_ops V2

[David Miller]

From: Linus Torvalds 
Date: Wed, 28 Nov 2018 10:00:06 -0800

> Not all memory is accessible even to the kernel. If you have memory
> that shows up in the last page of phys_addr_t, you just mark it
> reserved at boot-time.

It's not the physical memory at the end that needs to be reserved.

It's the IOMMU mapping arena.

That basically requires changes to every IOMMU implementation in
the tree.  Not saying it's hard or impossible, but it is real
meticulous work that needs to be done in order to realize this.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/netfront: remove unnecessary wmb

[David Miller]

From: Jacob Wen 
Date: Fri,  9 Nov 2018 14:53:59 +0800

> RING_PUSH_REQUESTS_AND_CHECK_NOTIFY is already able to make sure backend sees
> requests before req_prod is updated.
> 
> Signed-off-by: Jacob Wen 

Applied to net-next.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next] net: xen-netback: fix return type of ndo_start_xmit function

[David Miller]

From: YueHaibing 
Date: Wed, 26 Sep 2018 17:18:14 +0800

> The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
> which is a typedef for an enum type, so make sure the implementation in
> this driver has returns 'netdev_tx_t' value, and change the function
> return type to netdev_tx_t.
> 
> Found by coccinelle.
> 
> Signed-off-by: YueHaibing 
> Acked-by: Wei Liu 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net 0/3 RESEND] xen-netback: hash mapping handling adjustments

[David Miller]

From: "Jan Beulich" 
Date: Tue, 25 Sep 2018 02:11:33 -0600

> First and foremost the fix for XSA-270. On top of that further changes
> which looked desirable to me while investigating that XSA.
> 
> 1: fix input validation in xenvif_set_hash_mapping()
> 2: validate queue numbers in xenvif_set_hash_mapping()
> 3: handle page straddling in xenvif_set_hash_mapping()
> 
> Signed-off-by: Jan Beulich 

Series applied and queued up for -stable.

Thanks.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Ping: [PATCH 0/3] xen-netback: hash mapping hanling adjustments

[David Miller]

From: "Jan Beulich" 
Date: Mon, 24 Sep 2018 01:43:50 -0600

 On 11.09.18 at 12:16,  wrote:
>> On Tue, Sep 11, 2018 at 02:12:07AM -0600, Jan Beulich wrote:
>>> >>> On 28.08.18 at 16:54,  wrote:
>>> > First and foremost the fix for XSA-270. On top of that further changes
>>> > which looked desirable to me while investigating that XSA.
>>> > 
>>> > 1: fix input validation in xenvif_set_hash_mapping()
>>> > 2: validate queue numbers in xenvif_set_hash_mapping()
>>> > 3: handle page straddling in xenvif_set_hash_mapping()
>>> > 
>>> > Signed-off-by: Jan Beulich 
>>> 
>>> What is the way forward here? I've got R-b-s from Paul for all three
>>> patches, and a minor change request on patch 2 from Wei. I'm not
>>> really certain what to do in this case (hints appreciated), but could
>>> at least the security fix (patch 1) be applied immediately?
>> 
>> If you happen to resend, please make the adjustment; otherwise I'm fine
>> with the patches as they are. I don't want to block useful things on
>> cosmetic issues.
> 
> Dave? I notice none of the patches is in 4.19-rc5, not even the security
> fix, the advisory for which had gone public over a month ago.

If it's not in my patchwork queue, you have to resend the series and
make it clear that it should be applied to the networking tree by
putting "[PATCH net N/M]" in the Subject lines.

Thank you.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next 00/22] net: fix return type of ndo_start_xmit function

[David Miller]

From: YueHaibing 
Date: Thu, 20 Sep 2018 20:32:44 +0800

> The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
> which is a typedef for an enum type, so make sure the implementation in
> this driver has returns 'netdev_tx_t' value, and change the function
> return type to netdev_tx_t.

I would advise you not to send so many of these changes as a group.

If one of the patches needs feedback addressed, which is already the
case, you will have to resubmit the entire series all over again with
the fixes.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/netfront: don't bug in case of too many frags

[David Miller]

From: Juergen Gross 
Date: Tue, 11 Sep 2018 09:04:48 +0200

> Commit 57f230ab04d291 ("xen/netfront: raise max number of slots in
> xennet_get_responses()") raised the max number of allowed slots by one.
> This seems to be problematic in some configurations with netback using
> a larger MAX_SKB_FRAGS value (e.g. old Linux kernel with MAX_SKB_FRAGS
> defined as 18 instead of nowadays 17).
> 
> Instead of BUG_ON() in this case just fall back to retransmission.
> 
> Fixes: 57f230ab04d291 ("xen/netfront: raise max number of slots in 
> xennet_get_responses()")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Juergen Gross 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen-netback: remove unecessary condition check before debugfs_remove_recursive

[David Miller]

From: zhong jiang 
Date: Sat, 8 Sep 2018 21:53:42 +0800

> debugfs_remove_recursive has taken IS_ERR_OR_NULL into account. So just
> remove the condition check before debugfs_remove_recursive.
> 
> Signed-off-by: zhong jiang 

Applied to net-next.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] net: xenbus: remove redundant condition check before debugfs_remove_recursive

[David Miller]

From: zhong jiang 
Date: Sat, 8 Sep 2018 21:35:06 +0800

> debugfs_remove_recursive has taken the IS_ERR_OR_NULL into account. Just
> remove the unnecessary condition check.
> 
> Signed-off-by: zhong jiang 

Applied to net-next.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/netfront: fix waiting for xenbus state change

[David Miller]

From: Juergen Gross 
Date: Fri,  7 Sep 2018 14:21:30 +0200

> Commit 822fb18a82aba ("xen-netfront: wait xenbus state change when load
> module manually") added a new wait queue to wait on for a state change
> when the module is loaded manually. Unfortunately there is no wakeup
> anywhere to stop that waiting.
> 
> Instead of introducing a new wait queue rename the existing
> module_unload_q to module_wq and use it for both purposes (loading and
> unloading).
> 
> As any state change of the backend might be intended to stop waiting
> do the wake_up_all() in any case when netback_changed() is called.
> 
> Fixes: 822fb18a82aba ("xen-netfront: wait xenbus state change when load 
> module manually")
> Cc:  #4.18
> Signed-off-by: Juergen Gross 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next v3] xen-netfront: fix warn message as irq device name has '/'

[David Miller]

From: Xiao Liang 
Date: Tue, 14 Aug 2018 23:21:28 +0800

> There is a call trace generated after commit 
> 2d408c0d4574b01b9ed45e02516888bf925e11a9(
> xen-netfront: fix queue name setting). There is no 'device/vif/xx-q0-tx' file 
> found
> under /proc/irq/xx/.
> 
> This patch only picks up device type and id as its name.
> 
> With the patch, now /proc/interrupts looks like below and the warning message 
> gone:
>  70: 21  0  0  0   xen-dyn-event 
> vif0-q0-tx
>  71: 15  0  0  0   xen-dyn-event 
> vif0-q0-rx
>  72: 14  0  0  0   xen-dyn-event 
> vif0-q1-tx
>  73: 33  0  0  0   xen-dyn-event 
> vif0-q1-rx
>  74: 12  0  0  0   xen-dyn-event 
> vif0-q2-tx
>  75: 24  0  0  0   xen-dyn-event 
> vif0-q2-rx
>  76: 19  0  0  0   xen-dyn-event 
> vif0-q3-tx
>  77: 21  0  0  0   xen-dyn-event 
> vif0-q3-rx
> 
> Below is call trace information without this patch:
 ...
> Signed-off-by: Xiao Liang 
> Reviewed-by: Juergen Gross 

Applied, thank you.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] xen-netfront: fix warn message as irq device name has '/'

[David Miller]

From: Xiao Liang 
Date: Sat, 11 Aug 2018 23:21:37 +0800

> There is a call trace generated after commit 
> 2d408c0d4574b01b9ed45e02516888bf925e11a9(
> xen-netfront: fix queue name setting). There is no 'device/vif/xx-q0-tx' file 
> found
> under /proc/irq/xx/.
> 
> This patch only picks up device type and id as its name.

This adds a compile warning:

drivers/net/xen-netfront.c: In function ‘xennet_init_queue’:
drivers/net/xen-netfront.c:1614:2: warning: ignoring return value of 
‘kstrtoint’, declared with attribute warn_unused_result [-Wunused-result]
  kstrtoint(strrchr(queue->info->xbdev->nodename, '/') + 1, 10, );
  ^
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/netfront: don't cache skb_shinfo()

[David Miller]

From: Juergen Gross 
Date: Thu,  9 Aug 2018 16:42:16 +0200

> skb_shinfo() can change when calling __pskb_pull_tail(): Don't cache
> its return value.
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Juergen Gross 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next] xen-netback: use true and false for boolean values

[David Miller]

From: "Gustavo A. R. Silva" 
Date: Wed, 1 Aug 2018 19:31:01 -0500

> Return statements in functions returning bool should use true or false
> instead of an integer value.
> 
> This issue was detected with the help of Coccinelle.
> 
> Signed-off-by: Gustavo A. R. Silva 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen-netfront: wait xenbus state change when load module manually

[David Miller]

From: Xiao Liang 
Date: Fri, 27 Jul 2018 17:56:08 +0800

> When loading module manually, after call xenbus_switch_state to initializes
> the state of the netfront device, the driver state did not change so fast
> that may lead no dev created in latest kernel. This patch adds wait to make
> sure xenbus knows the driver is not in closed/unknown state.
 ...
> Signed-off-by: Xiao Liang 

Applied and queued up for -stable, thanks.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen-netfront: wait xenbus state change when load module manually

[David Miller]

From: Xiao Liang 
Date: Fri, 27 Jul 2018 17:56:08 +0800

> @@ -1330,6 +1331,11 @@ static struct net_device *xennet_create_dev(struct 
> xenbus_device *dev)
>   netif_carrier_off(netdev);
>  
>   xenbus_switch_state(dev, XenbusStateInitialising);
> + wait_event(module_load_q,
> +xenbus_read_driver_state(dev->otherend) !=
> +XenbusStateClosed &&
> +xenbus_read_driver_state(dev->otherend) !=
> +XenbusStateUnknown);
>   return netdev;
>  
>   exit:

What performs the wakeups that will trigger for this sleep site?

Thank you.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next] xen-netfront: fix queue name setting

[David Miller]

From: Vitaly Kuznetsov 
Date: Fri, 20 Jul 2018 18:33:59 +0200

> Commit f599c64fdf7d ("xen-netfront: Fix race between device setup and
> open") changed the initialization order: xennet_create_queues() now
> happens before we do register_netdev() so using netdev->name in
> xennet_init_queue() is incorrect, we end up with the following in
> /proc/interrupts:
> 
>  60:139  0   xen-dyn-event eth%d-q0-tx
>  61:265  0   xen-dyn-event eth%d-q0-rx
>  62:234  0   xen-dyn-event eth%d-q1-tx
>  63:  1  0   xen-dyn-event eth%d-q1-rx
> 
> and this looks ugly. Actually, using early netdev name (even when it's
> already set) is also not ideal: nowadays we tend to rename eth devices
> and queue name may end up not corresponding to the netdev name.
> 
> Use nodename from xenbus device for queue naming: this can't change in VM's
> lifetime. Now /proc/interrupts looks like
> 
>  62:202  0   xen-dyn-event device/vif/0-q0-tx
>  63:317  0   xen-dyn-event device/vif/0-q0-rx
>  64:262  0   xen-dyn-event device/vif/0-q1-tx
>  65: 17  0   xen-dyn-event device/vif/0-q1-rx
> 
> Fixes: f599c64fdf7d ("xen-netfront: Fix race between device setup and open")
> Signed-off-by: Vitaly Kuznetsov 

Patch applied, thank you.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/2] xen-netfront: Fix issues with commit f599c64fdf7d

[David Miller]

From: Ross Lagerwall 
Date: Thu, 21 Jun 2018 14:00:19 +0100

> Fix a couple of issues with commit f599c64fdf7d ("xen-netfront: Fix race
> between device setup and open").

Series applied and queued up for -stable.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/1] xen-netback: process malformed sk_buff correctly to avoid BUG_ON()

[David Miller]

From: Dongli Zhang 
Date: Wed, 28 Mar 2018 07:42:16 +0800

> The "BUG_ON(!frag_iter)" in function xenvif_rx_next_chunk() is triggered if
> the received sk_buff is malformed, that is, when the sk_buff has pattern
> (skb->data_len && !skb_shinfo(skb)->nr_frags). Below is a sample call
> stack:

We should fix the parts of the kernel which build illegal malformed
SKBs rather than adding checks to every driver in the tree.

I'm not applying this, sorry.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] drivers/net: Use octal not symbolic permissions

[David Miller]

From: Joe Perches 
Date: Fri, 23 Mar 2018 15:54:39 -0700

> Prefer the direct use of octal for permissions.
> 
> Done with checkpatch -f --types=SYMBOLIC_PERMS --fix-inplace
> and some typing.
> 
> Miscellanea:
> 
> o Whitespace neatening around these conversions.
> 
> Signed-off-by: Joe Perches 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH][next] xen-netback: make function xenvif_rx_skb static

[David Miller]

From: Colin King 
Date: Fri, 23 Feb 2018 17:16:57 +

> From: Colin Ian King 
> 
> The function xenvif_rx_skb is local to the source and does not need
> to be in global scope, so make it static.
> 
> Cleans up sparse warning:
> drivers/net/xen-netback/rx.c:422:6: warning: symbol 'xenvif_rx_skb'
> was not declared. Should it be static?
> 
> Signed-off-by: Colin Ian King 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] xen-netfront: Fix race between device setup and open

[David Miller]

From: Ross Lagerwall 
Date: Thu, 11 Jan 2018 15:49:07 +

> I've now added CC'd netdev on the other two.

That doesn't work.

If you don't post the originals explicitly to netdev then it won't
get properly queued in patchwork.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] xen-netfront: Fix race between device setup and open

[David Miller]

From: Ross Lagerwall 
Date: Thu, 11 Jan 2018 09:36:38 +

> When a netfront device is set up it registers a netdev fairly early on,
> before it has set up the queues and is actually usable. A userspace tool
> like NetworkManager will immediately try to open it and access its state
> as soon as it appears. The bug can be reproduced by hotplugging VIFs
> until the VM runs out of grant refs. It registers the netdev but fails
> to set up any queues (since there are no more grant refs). In the
> meantime, NetworkManager opens the device and the kernel crashes trying
> to access the queues (of which there are none).
> 
> Fix this in two ways:
> * For initial setup, register the netdev much later, after the queues
> are setup. This avoids the race entirely.
> * During a suspend/resume cycle, the frontend reconnects to the backend
> and the queues are recreated. It is possible (though highly unlikely) to
> race with something opening the device and accessing the queues after
> they have been destroyed but before they have been recreated. Extend the
> region covered by the rtnl semaphore to protect against this race. There
> is a possibility that we fail to recreate the queues so check for this
> in the open function.
> 
> Signed-off-by: Ross Lagerwall 

Where is patch 1/2 and the 0/2 header posting which explains what this
patch series is doing, how it is doing it, and why it is doing it that
way?

Thanks.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen-netfront: enable device after manual module load

[David Miller]

From: Eduardo Otubo 
Date: Fri,  5 Jan 2018 09:42:16 +0100

> When loading the module after unloading it, the network interface would
> not be enabled and thus wouldn't have a backend counterpart and unable
> to be used by the guest.
> 
> The guest would face errors like:
> 
>   [root@guest ~]# ethtool -i eth0
>   Cannot get driver information: No such device
> 
>   [root@guest ~]# ifconfig eth0
>   eth0: error fetching interface information: Device not found
> 
> This patch initializes the state of the netfront device whenever it is
> loaded manually, this state would communicate the netback to create its
> device and establish the connection between them.
> 
> Signed-off-by: Eduardo Otubo 

Applied, thank you.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH net-next v2] xen-netback: make copy batch size configurable

[David Miller]

From: Joao Martins 
Date: Thu, 21 Dec 2017 17:24:28 +

> Commit eb1723a29b9a ("xen-netback: refactor guest rx") refactored Rx
> handling and as a result decreased max grant copy ops from 4352 to 64.
> Before this commit it would drain the rx_queue (while there are
> enough slots in the ring to put packets) then copy to all pages and write
> responses on the ring. With the refactor we do almost the same albeit
> the last two steps are done every COPY_BATCH_SIZE (64) copies.
> 
> For big packets, the value of 64 means copying 3 packets best case scenario
> (17 copies) and worst-case only 1 packet (34 copies, i.e. if all frags
> plus head cross the 4k grant boundary) which could be the case when
> packets go from local backend process.
> 
> Instead of making it static to 64 grant copies, lets allow the user to
> select its value (while keeping the current as default) by introducing
> the `copy_batch_size` module parameter. This allows users to select
> the higher batches (i.e. for better throughput with big packets) as it
> was prior to the above mentioned commit.
> 
> Signed-off-by: Joao Martins 
> ---
> Changes since v1:
>  * move rx_copy.{idx,op} reallocation to separate helper
>  Addressed Paul's comments:
>  * rename xenvif_copy_state#size field to batch_size
>  * argument `size` should be unsigned int
>  * vfree is safe with NULL
>  * realloc rx_copy.{idx,op} after copy op flush

I truly dislike things of this nature.

When you give the user a numerical value to set, they have to pick
something.  This in turn requires deep, weird, knowledge of how the
driver implements RX packet processing.

That's asbolutely unacceptable.  Can you imagine being an admin and
trying to figure out what random number to plug into this thing?

"maximum number of grant copies on RX"

I've been the networking maintainer for more than 2 decades and I
have no idea whatsoever what kind of value I might want to set
there.

Nobody should have to know this other than people working on the
driver.

Instead, the issue is that the driver can optimize for throughput
or something else (latency, RX packing, I don't know exactly what
it is, but you're keeping the default value so it has some merit
right?).  Therefore, what you need to export is a boolean which
is self describing.

"rx_optimize_throughput"

That's it.  And you, the smart person who knows what this grant
copy mumbo jumbo means, can pick a specific value to use for
high throughput.

Thank you.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen-netback: Fix logging message with spurious period after newline

[David Miller]

From: Joe Perches 
Date: Tue,  5 Dec 2017 22:40:25 -0800

> Using a period after a newline causes bad output.
> 
> Signed-off-by: Joe Perches 

Applied.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCHv2] xen-netfront: remove warning when unloading module

[David Miller]

From: Eduardo Otubo 
Date: Thu, 23 Nov 2017 15:18:35 +0100

> v2:
>  * Replace busy wait with wait_event()/wake_up_all()
>  * Cannot garantee that at the time xennet_remove is called, the
>xen_netback state will not be XenbusStateClosed, so added a
>condition for that
>  * There's a small chance for the xen_netback state is
>XenbusStateUnknown by the time the xen_netfront switches to Closed,
>so added a condition for that.
> 
> When unloading module xen_netfront from guest, dmesg would output
> warning messages like below:
> 
>   [  105.236836] xen:grant_table: WARNING: g.e. 0x903 still in use!
>   [  105.236839] deferring g.e. 0x903 (pfn 0x35805)
> 
> This problem relies on netfront and netback being out of sync. By the time
> netfront revokes the g.e.'s netback didn't have enough time to free all of
> them, hence displaying the warnings on dmesg.
> 
> The trick here is to make netfront to wait until netback frees all the g.e.'s
> and only then continue to cleanup for the module removal, and this is done by
> manipulating both device states.
> 
> Signed-off-by: Eduardo Otubo 

Applied, thank you.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel