* [PATCH 2/3] tun: free skb in early errors
From: wexu @ 2017-12-01 5:54 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: mjrosato, wexu, mst
In-Reply-To: <1512107669-27572-1-git-send-email-wexu@redhat.com>
From: Wei Xu <wexu@redhat.com>
tun_recvmsg() supports accepting skb by msg_control after
commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
the skb if presented should be freed within the function, otherwise it
would be leaked.
Signed-off-by: Wei Xu <wexu@redhat.com>
Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
drivers/net/tun.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 6a7bde9..5563430 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2067,14 +2067,17 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
{
struct tun_file *tfile = container_of(sock, struct tun_file, socket);
struct tun_struct *tun = tun_get(tfile);
+ struct sk_buff *skb = m->msg_control;
int ret;
- if (!tun)
- return -EBADFD;
+ if (!tun) {
+ ret = -EBADFD;
+ goto out_free_skb;
+ }
if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
ret = -EINVAL;
- goto out;
+ goto out_free_skb;
}
if (flags & MSG_ERRQUEUE) {
ret = sock_recv_errqueue(sock->sk, m, total_len,
@@ -2087,6 +2090,11 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
m->msg_flags |= MSG_TRUNC;
ret = flags & MSG_TRUNC ? ret : total_len;
}
+ goto out;
+
+out_free_skb:
+ if (skb)
+ kfree_skb(skb);
out:
tun_put(tun);
return ret;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 3/3] tap: free skb if flags error
From: wexu @ 2017-12-01 5:54 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: mjrosato, wexu, mst
In-Reply-To: <1512107669-27572-1-git-send-email-wexu@redhat.com>
From: Wei Xu <wexu@redhat.com>
tap_recvmsg() supports accepting skb by msg_control after
commit 3b4ba04acca8 ("tap: support receiving skb from msg_control"),
the skb if presented should be freed within the function, otherwise
it would be leaked.
Signed-off-by: Wei Xu <wexu@redhat.com>
Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
drivers/net/tap.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index e9489b8..1c66b75 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -1154,9 +1154,13 @@ static int tap_recvmsg(struct socket *sock, struct msghdr *m,
size_t total_len, int flags)
{
struct tap_queue *q = container_of(sock, struct tap_queue, sock);
+ struct sk_buff *skb = m->msg_control;
int ret;
- if (flags & ~(MSG_DONTWAIT|MSG_TRUNC))
+ if (flags & ~(MSG_DONTWAIT|MSG_TRUNC)) {
+ if (skb)
+ kfree_skb(skb);
return -EINVAL;
+ }
ret = tap_do_read(q, &m->msg_iter, flags & MSG_DONTWAIT,
m->msg_control);
if (ret > total_len) {
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 2/3] tun: free skb in early errors
From: Jason Wang @ 2017-12-01 7:07 UTC (permalink / raw)
To: wexu, virtualization, netdev, linux-kernel; +Cc: mjrosato, mst
In-Reply-To: <1512107669-27572-3-git-send-email-wexu@redhat.com>
On 2017年12月01日 13:54, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> tun_recvmsg() supports accepting skb by msg_control after
> commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
> the skb if presented should be freed within the function, otherwise it
> would be leaked.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
> drivers/net/tun.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 6a7bde9..5563430 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2067,14 +2067,17 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
> {
> struct tun_file *tfile = container_of(sock, struct tun_file, socket);
> struct tun_struct *tun = tun_get(tfile);
> + struct sk_buff *skb = m->msg_control;
> int ret;
>
> - if (!tun)
> - return -EBADFD;
> + if (!tun) {
> + ret = -EBADFD;
> + goto out_free_skb;
Unfortunately, you can't to there since tun is NULL.
> + }
>
> if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
> ret = -EINVAL;
> - goto out;
> + goto out_free_skb;
> }
> if (flags & MSG_ERRQUEUE) {
> ret = sock_recv_errqueue(sock->sk, m, total_len,
> @@ -2087,6 +2090,11 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
> m->msg_flags |= MSG_TRUNC;
> ret = flags & MSG_TRUNC ? ret : total_len;
> }
> + goto out;
We usually don't use goto in the case of success, and you need deal with
the case skb != NULL but iov_iter_count(to) == 0 in tun_do_read().
Thanks
> +
> +out_free_skb:
> + if (skb)
> + kfree_skb(skb);
> out:
> tun_put(tun);
> return ret;
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 3/3] tap: free skb if flags error
From: Jason Wang @ 2017-12-01 7:10 UTC (permalink / raw)
To: wexu, virtualization, netdev, linux-kernel; +Cc: mjrosato, mst
In-Reply-To: <1512107669-27572-4-git-send-email-wexu@redhat.com>
On 2017年12月01日 13:54, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> tap_recvmsg() supports accepting skb by msg_control after
> commit 3b4ba04acca8 ("tap: support receiving skb from msg_control"),
> the skb if presented should be freed within the function, otherwise
> it would be leaked.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
> drivers/net/tap.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index e9489b8..1c66b75 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -1154,9 +1154,13 @@ static int tap_recvmsg(struct socket *sock, struct msghdr *m,
> size_t total_len, int flags)
> {
> struct tap_queue *q = container_of(sock, struct tap_queue, sock);
> + struct sk_buff *skb = m->msg_control;
> int ret;
> - if (flags & ~(MSG_DONTWAIT|MSG_TRUNC))
> + if (flags & ~(MSG_DONTWAIT|MSG_TRUNC)) {
> + if (skb)
> + kfree_skb(skb);
> return -EINVAL;
> + }
> ret = tap_do_read(q, &m->msg_iter, flags & MSG_DONTWAIT,
> m->msg_control);
Need to deal with iov_iterator_count() == 0.
Thanks
> if (ret > total_len) {
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 1/3] vhost: fix skb leak in handle_rx()
From: Jason Wang @ 2017-12-01 7:11 UTC (permalink / raw)
To: wexu, virtualization, netdev, linux-kernel; +Cc: mjrosato, mst
In-Reply-To: <1512107669-27572-2-git-send-email-wexu@redhat.com>
On 2017年12月01日 13:54, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Matthew found a roughly 40% tcp throughput regression with commit
> c67df11f(vhost_net: try batch dequing from skb array) as discussed
> in the following thread:
> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
>
> Eventually we figured out that it was a skb leak in handle_rx()
> when sending packets to the VM. This usually happens when a guest
> can not drain out vq as fast as vhost fills in, afterwards it sets
> off the traffic jam and leaks skb(s) which occurs as no headcount
> to send on the vq from vhost side.
>
> This can be avoided by making sure we have got enough headcount
> before actually consuming a skb from the batched rx array while
> transmitting, which is simply done by moving checking the zero
> headcount a bit ahead.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
> drivers/vhost/net.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 8d626d7..c7bdeb6 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
> /* On error, stop handling until the next kick. */
> if (unlikely(headcount < 0))
> goto out;
> - if (nvq->rx_array)
> - msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> - /* On overrun, truncate and discard */
> - if (unlikely(headcount > UIO_MAXIOV)) {
> - iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> - err = sock->ops->recvmsg(sock, &msg,
> - 1, MSG_DONTWAIT | MSG_TRUNC);
> - pr_debug("Discarded rx packet: len %zd\n", sock_len);
> - continue;
> - }
> /* OK, now we need to know about added descriptors. */
> if (!headcount) {
> if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> @@ -800,6 +790,16 @@ static void handle_rx(struct vhost_net *net)
> * they refilled. */
> goto out;
> }
> + if (nvq->rx_array)
> + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> + /* On overrun, truncate and discard */
> + if (unlikely(headcount > UIO_MAXIOV)) {
> + iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> + err = sock->ops->recvmsg(sock, &msg,
> + 1, MSG_DONTWAIT | MSG_TRUNC);
> + pr_debug("Discarded rx packet: len %zd\n", sock_len);
> + continue;
> + }
> /* We don't need to be notified again. */
> iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
> fixup = msg.msg_iter;
I suggest to reorder this patch to 3/3.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
From: Daniel Vetter @ 2017-12-01 7:19 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: Teddy Wang, David Airlie, Greg KH, dri-devel,
Michał Mirosław, amd-gfx, Daniel Vetter, Alex Deucher,
virtualization, Christian König
In-Reply-To: <20171130234953.GA5542@sudip-laptop>
On Thu, Nov 30, 2017 at 11:49:53PM +0000, Sudip Mukherjee wrote:
> Hi Daniel,
>
> On Wed, Nov 29, 2017 at 10:56:34AM +0100, Daniel Vetter wrote:
> > On Tue, Nov 28, 2017 at 12:30:30PM +0000, Sudip Mukherjee wrote:
> > > On Tue, Nov 28, 2017 at 12:32:38PM +0100, Greg KH wrote:
> > > > On Tue, Nov 28, 2017 at 11:22:17AM +0100, Daniel Vetter wrote:
> > > > > On Mon, Nov 27, 2017 at 08:52:19PM +0000, Sudip Mukherjee wrote:
> > > > > > On Mon, Nov 27, 2017 at 11:27:59AM +0100, Daniel Vetter wrote:
> > > > > > > On Fri, Nov 24, 2017 at 06:53:31PM +0100, Michał Mirosław wrote:
> > > > > > > > Almost all drivers using remove_conflicting_framebuffers() wrap it with
> > > > > > > > the same code. Extract common part from PCI drivers into separate
> > > > > > > > remove_conflicting_pci_framebuffers().
> > > > > > > >
>
> <snip>
>
> > > > >
> > > > > Greg?
> > > >
> > > > Yes, if no one is working to get it out of staging, that means no one
> > > > cares about it, and it needs to be removed from the tree.
> > >
> > > Agreed. I was not working on getting it out of staging as there is no
> > > place for it to go.
> > > But, Teddy Wang told me that they have a working drm driver for it, but
> > > it is not atomic like Daniel was asking for. If it is ok, then I can send
> > > in a patch to remove the existing sm750 from staging and add the new sm750
> > > drm driver to staging. And after it is ready, we can go ahead with moving
> > > it out of staging to drm.
> >
> > Please keep the todo item that it needs to be converted to atomic. And
> > tbh, it's probably faster if you just submit it to dri-devel, assuming you
> > have time to work on it. For small drivers we tend to be fairly quick in
> > getting them into good enough shape.
>
> I have received the driver from Teddy and pushed it to
> https://github.com/sudipm-mukherjee/parport/tree/drm_smi for your first
> look into it. It is not even building with next-20171130 and has lots of
> build warnings. I will have to do a lot of work on it before I can even
> submit it to dri-devel.
>
> Time will be the problem, as this is not part of my day job.
>
> >
> > Staging is also a major pain for drm subsystem refactorings, I really,
> > really, really prefer we don't add more than the vbox pain we have
> > already.
>
> I am hoping that after seeing it, you will agree to have it in staging. :)
So I know Greg is willing to take anything into staging, but I'm no fan.
We refactor and improve drm a lot, with a lot of cross-driver changes
necessary to move things forward. We can do that since we have a generally
rather active development community, and we try hard to keep most drivers
in reasonable good shape and so easy to maintain.
If you know throw a pile of unmaintainable stuff into staging, but without
working on it, then that's just cost, no benefit to the dri-devel
community. On top, staging tree is separate from drm trees, so more pain
to synchronize trees (and we have to do that a few times per release cycle
or drivers simply stop compiling). Where's the upside of taking this
driver into staging?
One is users, but ime for soc display drivers usually everything else is
in worse shape (e.g. even great drivers like the one for qualcom must be
tested on some vendor tree because critical core bits are missing in
upstream), so "more users" is not the good reason. And it's clearly not
"more developers", because no time to clean things up. So what exactly is
the good reason to put this into staging instead of just waiting until
someone has the time to clean it up quickly?
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v18 05/10] xbitmap: add more operations
From: Wei Wang @ 2017-12-01 8:02 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, willy,
quan.xu, nilal, riel, cornelia.huck, mhocko, linux-kernel,
amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <201711301934.CDC21800.FSLtJFFOOVQHMO@I-love.SAKURA.ne.jp>
On 11/30/2017 06:34 PM, Tetsuo Handa wrote:
> Wei Wang wrote:
>> + * @start: the start of the bit range, inclusive
>> + * @end: the end of the bit range, inclusive
>> + *
>> + * This function is used to clear a bit in the xbitmap. If all the bits of the
>> + * bitmap are 0, the bitmap will be freed.
>> + */
>> +void xb_clear_bit_range(struct xb *xb, unsigned long start, unsigned long end)
>> +{
>> + struct radix_tree_root *root = &xb->xbrt;
>> + struct radix_tree_node *node;
>> + void **slot;
>> + struct ida_bitmap *bitmap;
>> + unsigned int nbits;
>> +
>> + for (; start < end; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
>> + unsigned long index = start / IDA_BITMAP_BITS;
>> + unsigned long bit = start % IDA_BITMAP_BITS;
>> +
>> + bitmap = __radix_tree_lookup(root, index, &node, &slot);
>> + if (radix_tree_exception(bitmap)) {
>> + unsigned long ebit = bit + 2;
>> + unsigned long tmp = (unsigned long)bitmap;
>> +
>> + nbits = min(end - start + 1, BITS_PER_LONG - ebit);
> "nbits = min(end - start + 1," seems to expect that start == end is legal
> for clearing only 1 bit. But this function is no-op if start == end.
> Please clarify what "inclusive" intended.
If xb_clear_bit_range(xb,10,10), then it is effectively the same as
xb_clear_bit(10). Why would it be illegal?
"@start inclusive" means that the @start will also be included to be
cleared.
>
>> +static inline __always_inline void bitmap_clear(unsigned long *map,
>> + unsigned int start,
>> + unsigned int nbits)
>> +{
>> + if (__builtin_constant_p(nbits) && nbits == 1)
>> + __clear_bit(start, map);
>> + else if (__builtin_constant_p(start & 7) && IS_ALIGNED(start, 8) &&
>> + __builtin_constant_p(nbits & 7) && IS_ALIGNED(nbits, 8))
> It looks strange to apply __builtin_constant_p test to variables after "& 7".
>
I think this is normal - if the variables are known at compile time, the
calculation will be done at compile time (termed constant folding).
Best,
Wei
^ permalink raw reply
* Re: [PATCH v18 06/10] virtio_ring: add a new API, virtqueue_add_one_desc
From: Wei Wang @ 2017-12-01 8:06 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <20171130213231-mutt-send-email-mst@kernel.org>
On 12/01/2017 03:38 AM, Michael S. Tsirkin wrote:
> On Wed, Nov 29, 2017 at 09:55:22PM +0800, Wei Wang wrote:
>> Current virtqueue_add API implementation is based on the scatterlist
>> struct, which uses kaddr. This is inadequate to all the use case of
>> vring. For example:
>> - Some usages don't use IOMMU, in this case the user can directly pass
>> in a physical address in hand, instead of going through the sg
>> implementation (e.g. the VIRTIO_BALLOON_F_SG feature)
>> - Sometimes, a guest physical page may not have a kaddr (e.g. high
>> memory) but need to use vring (e.g. the VIRTIO_BALLOON_F_FREE_PAGE_VQ
>> feature)
>>
>> The new API virtqueue_add_one_desc enables the caller to assign a vring
>> desc with a physical address and len. Also, factor out the common code
>> with virtqueue_add in vring_set_avail.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
> You previously managed without this patch, and it's preferable
> IMHO since this patchset is already too big.
>
> I don't really understand what is wrong with virtio_add_sgs + sg_set_page.
> I don't think is assumes a kaddr.
>
OK, I will use the previous method to send sgs.
Please have a check if there are other things need to be improved.
Best,
Wei
^ permalink raw reply
* [PATCH] virtio_balloon: fix increment of vb->num_pfns in fill_balloon()
From: Jan Stancek @ 2017-12-01 9:50 UTC (permalink / raw)
To: mst, jasowang, virtualization
Cc: penguin-kernel, mhocko, linux-kernel, jstancek
commit c7cdff0e8647 ("virtio_balloon: fix deadlock on OOM")
changed code to increment vb->num_pfns before call to
set_page_pfns(), which used to happen only after.
This patch fixes boot hang for me on ppc64le KVM guests.
Fixes: c7cdff0e8647 ("virtio_balloon: fix deadlock on OOM")
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
drivers/virtio/virtio_balloon.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 7960746f7597..a1fb52cb3f0a 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -174,13 +174,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
while ((page = balloon_page_pop(&pages))) {
balloon_page_enqueue(&vb->vb_dev_info, page);
- vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
-
set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
if (!virtio_has_feature(vb->vdev,
VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
adjust_managed_page_count(page, -1);
+ vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
}
num_allocated_pages = vb->num_pfns;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net,stable v4 0/3] vhost: fix a few skb leaks
From: wexu @ 2017-12-01 10:10 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: mjrosato, wexu, mst
From: Wei Xu <wexu@redhat.com>
Matthew found a roughly 40% tcp throughput regression with commit
c67df11f(vhost_net: try batch dequing from skb array) as discussed
in the following thread:
https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
v4:
- fix zero iov iterator count in tap/tap_do_read()(Jason)
- don't put tun in case of EBADFD(Jason)
- Replace msg->msg_control with new 'skb' when calling tun/tap_do_read()
v3:
- move freeing skb from vhost to tun/tap recvmsg() to not
confuse the callers.
v2:
- add Matthew as the reporter, thanks matthew.
- moving zero headcount check ahead instead of defer consuming skb
due to jason and mst's comment.
- add freeing skb in favor of recvmsg() fails.
Wei Xu (3):
vhost: fix skb leak in handle_rx()
tun: free skb in early errors
tap: free skb if flags error
drivers/net/tap.c | 14 ++++++++++----
drivers/net/tun.c | 24 ++++++++++++++++++------
drivers/vhost/net.c | 20 ++++++++++----------
3 files changed, 38 insertions(+), 20 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH 1/3] vhost: fix skb leak in handle_rx()
From: wexu @ 2017-12-01 10:10 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: mjrosato, wexu, mst
In-Reply-To: <1512123038-15773-1-git-send-email-wexu@redhat.com>
From: Wei Xu <wexu@redhat.com>
Matthew found a roughly 40% tcp throughput regression with commit
c67df11f(vhost_net: try batch dequing from skb array) as discussed
in the following thread:
https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
Eventually we figured out that it was a skb leak in handle_rx()
when sending packets to the VM. This usually happens when a guest
can not drain out vq as fast as vhost fills in, afterwards it sets
off the traffic jam and leaks skb(s) which occurs as no headcount
to send on the vq from vhost side.
This can be avoided by making sure we have got enough headcount
before actually consuming a skb from the batched rx array while
transmitting, which is simply done by moving checking the zero
headcount a bit ahead.
Signed-off-by: Wei Xu <wexu@redhat.com>
Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
drivers/vhost/net.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 8d626d7..c7bdeb6 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
/* On error, stop handling until the next kick. */
if (unlikely(headcount < 0))
goto out;
- if (nvq->rx_array)
- msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
- /* On overrun, truncate and discard */
- if (unlikely(headcount > UIO_MAXIOV)) {
- iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
- err = sock->ops->recvmsg(sock, &msg,
- 1, MSG_DONTWAIT | MSG_TRUNC);
- pr_debug("Discarded rx packet: len %zd\n", sock_len);
- continue;
- }
/* OK, now we need to know about added descriptors. */
if (!headcount) {
if (unlikely(vhost_enable_notify(&net->dev, vq))) {
@@ -800,6 +790,16 @@ static void handle_rx(struct vhost_net *net)
* they refilled. */
goto out;
}
+ if (nvq->rx_array)
+ msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
+ /* On overrun, truncate and discard */
+ if (unlikely(headcount > UIO_MAXIOV)) {
+ iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
+ err = sock->ops->recvmsg(sock, &msg,
+ 1, MSG_DONTWAIT | MSG_TRUNC);
+ pr_debug("Discarded rx packet: len %zd\n", sock_len);
+ continue;
+ }
/* We don't need to be notified again. */
iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
fixup = msg.msg_iter;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/3] tun: free skb in early errors
From: wexu @ 2017-12-01 10:10 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: mjrosato, wexu, mst
In-Reply-To: <1512123038-15773-1-git-send-email-wexu@redhat.com>
From: Wei Xu <wexu@redhat.com>
tun_recvmsg() supports accepting skb by msg_control after
commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
the skb if presented should be freed no matter how far it can go
along, otherwise it would be leaked.
This patch fixes several missed cases.
Signed-off-by: Wei Xu <wexu@redhat.com>
Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
drivers/net/tun.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9574900..4f4a842 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1952,8 +1952,11 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
tun_debug(KERN_INFO, tun, "tun_do_read\n");
- if (!iov_iter_count(to))
+ if (!iov_iter_count(to)) {
+ if (skb)
+ kfree_skb(skb);
return 0;
+ }
if (!skb) {
/* Read frames from ring */
@@ -2069,22 +2072,24 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
{
struct tun_file *tfile = container_of(sock, struct tun_file, socket);
struct tun_struct *tun = tun_get(tfile);
+ struct sk_buff *skb = m->msg_control;
int ret;
- if (!tun)
- return -EBADFD;
+ if (!tun) {
+ ret = -EBADFD;
+ goto out_free_skb;
+ }
if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
ret = -EINVAL;
- goto out;
+ goto out_put_tun;
}
if (flags & MSG_ERRQUEUE) {
ret = sock_recv_errqueue(sock->sk, m, total_len,
SOL_PACKET, TUN_TX_TIMESTAMP);
goto out;
}
- ret = tun_do_read(tun, tfile, &m->msg_iter, flags & MSG_DONTWAIT,
- m->msg_control);
+ ret = tun_do_read(tun, tfile, &m->msg_iter, flags & MSG_DONTWAIT, skb);
if (ret > (ssize_t)total_len) {
m->msg_flags |= MSG_TRUNC;
ret = flags & MSG_TRUNC ? ret : total_len;
@@ -2092,6 +2097,13 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
out:
tun_put(tun);
return ret;
+
+out_put_tun:
+ tun_put(tun);
+out_free_skb:
+ if (skb)
+ kfree_skb(skb);
+ return ret;
}
static int tun_peek_len(struct socket *sock)
--
1.8.3.1
^ permalink raw reply related
* [PATCH 3/3] tap: free skb if flags error
From: wexu @ 2017-12-01 10:10 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: mjrosato, wexu, mst
In-Reply-To: <1512123038-15773-1-git-send-email-wexu@redhat.com>
From: Wei Xu <wexu@redhat.com>
tap_recvmsg() supports accepting skb by msg_control after
commit 3b4ba04acca8 ("tap: support receiving skb from msg_control"),
the skb if presented should be freed within the function, otherwise
it would be leaked.
Signed-off-by: Wei Xu <wexu@redhat.com>
Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
drivers/net/tap.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index e9489b8..0a886fda 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -829,8 +829,11 @@ static ssize_t tap_do_read(struct tap_queue *q,
DEFINE_WAIT(wait);
ssize_t ret = 0;
- if (!iov_iter_count(to))
+ if (!iov_iter_count(to)) {
+ if (skb)
+ kfree_skb(skb);
return 0;
+ }
if (skb)
goto put;
@@ -1154,11 +1157,14 @@ static int tap_recvmsg(struct socket *sock, struct msghdr *m,
size_t total_len, int flags)
{
struct tap_queue *q = container_of(sock, struct tap_queue, sock);
+ struct sk_buff *skb = m->msg_control;
int ret;
- if (flags & ~(MSG_DONTWAIT|MSG_TRUNC))
+ if (flags & ~(MSG_DONTWAIT|MSG_TRUNC)) {
+ if (skb)
+ kfree_skb(skb);
return -EINVAL;
- ret = tap_do_read(q, &m->msg_iter, flags & MSG_DONTWAIT,
- m->msg_control);
+ }
+ ret = tap_do_read(q, &m->msg_iter, flags & MSG_DONTWAIT, skb);
if (ret > total_len) {
m->msg_flags |= MSG_TRUNC;
ret = flags & MSG_TRUNC ? ret : total_len;
--
1.8.3.1
^ permalink raw reply related
* Memory corruption in powerpc guests with virtio_balloon (was Re: [PATCH v3] virtio_balloon: fix deadlock on OOM)
From: Michael Ellerman @ 2017-12-01 12:31 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Michal Hocko, Tetsuo Handa, virtualization, linux-mm,
linuxppc-dev
In-Reply-To: <1510154064-9709-1-git-send-email-mst@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> fill_balloon doing memory allocations under balloon_lock
> can cause a deadlock when leak_balloon is called from
> virtballoon_oom_notify and tries to take same lock.
>
> To fix, split page allocation and enqueue and do allocations outside the lock.
>
> Here's a detailed analysis of the deadlock by Tetsuo Handa:
>
> In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
> serialize against fill_balloon(). But in fill_balloon(),
> alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
> called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE]
> implies __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, despite __GFP_NORETRY
> is specified, this allocation attempt might indirectly depend on somebody
> else's __GFP_DIRECT_RECLAIM memory allocation. And such indirect
> __GFP_DIRECT_RECLAIM memory allocation might call leak_balloon() via
> virtballoon_oom_notify() via blocking_notifier_call_chain() callback via
> out_of_memory() when it reached __alloc_pages_may_oom() and held oom_lock
> mutex. Since vb->balloon_lock mutex is already held by fill_balloon(), it
> will cause OOM lockup.
>
> Thread1 Thread2
> fill_balloon()
> takes a balloon_lock
> balloon_page_enqueue()
> alloc_page(GFP_HIGHUSER_MOVABLE)
> direct reclaim (__GFP_FS context) takes a fs lock
> waits for that fs lock alloc_page(GFP_NOFS)
> __alloc_pages_may_oom()
> takes the oom_lock
> out_of_memory()
> blocking_notifier_call_chain()
> leak_balloon()
> tries to take that balloon_lock and deadlocks
>
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> drivers/virtio/virtio_balloon.c | 24 +++++++++++++++++++-----
> include/linux/balloon_compaction.h | 35 ++++++++++++++++++++++++++++++++++-
> mm/balloon_compaction.c | 28 +++++++++++++++++++++-------
> 3 files changed, 74 insertions(+), 13 deletions(-)
Somehow this commit seems to be killing powerpc guests.
The symptom is that the first page (64K) of the guests memory gets over
written with zeroes, which is where our interrupt handlers are, so the
system rapidly locks up due to illegal instructions in the illegal
instruction handler.
There seems to be some element of a race, because it doesn't always
crash. Sometimes I can boot to a shell, but not often. When it does
happen it's fairly late in boot, but before I get to a shell.
I had a few bisects go off into the weeds due to the intermittent
nature. But once I realised that I changed my script to boot 5 times
before declaring a kernel good, and that bisected straight here.
I can also revert this commit on v4.15-rc1 and everything's fine again -
I got through ~250 boots with that kernel.
So I'm pretty sure this commit is triggering/exposing/causing the bug.
The other data point is that the page that's overwritten is mapped read
only in the guest kernel. So either the guest kernel is writing to it in
real mode (MMU off), or the hypervisor/DMA is doing it.
I haven't isolated if it's host kernel/qemu version dependent, at the
moment I'm just using distro packaged versions of both.
Anyway I'll try and dig further into it on Monday, but I thought I'd let
you know in case this is a known bug with a fix in the pipeline, or
rings any bells or whatever.
cheers
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f0b3a0b..7960746 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -143,16 +143,17 @@ static void set_page_pfns(struct virtio_balloon *vb,
>
> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> - struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> unsigned num_allocated_pages;
> + unsigned num_pfns;
> + struct page *page;
> + LIST_HEAD(pages);
>
> /* We can only do one array worth at a time. */
> num = min(num, ARRAY_SIZE(vb->pfns));
>
> - mutex_lock(&vb->balloon_lock);
> - for (vb->num_pfns = 0; vb->num_pfns < num;
> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - struct page *page = balloon_page_enqueue(vb_dev_info);
> + for (num_pfns = 0; num_pfns < num;
> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> + struct page *page = balloon_page_alloc();
>
> if (!page) {
> dev_info_ratelimited(&vb->vdev->dev,
> @@ -162,6 +163,19 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> msleep(200);
> break;
> }
> +
> + balloon_page_push(&pages, page);
> + }
> +
> + mutex_lock(&vb->balloon_lock);
> +
> + vb->num_pfns = 0;
> +
> + while ((page = balloon_page_pop(&pages))) {
> + balloon_page_enqueue(&vb->vb_dev_info, page);
> +
> + vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
> +
> set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> if (!virtio_has_feature(vb->vdev,
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> index fbbe6da..c4c8df9 100644
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -50,6 +50,7 @@
> #include <linux/gfp.h>
> #include <linux/err.h>
> #include <linux/fs.h>
> +#include <linux/list.h>
>
> /*
> * Balloon device information descriptor.
> @@ -67,7 +68,9 @@ struct balloon_dev_info {
> struct inode *inode;
> };
>
> -extern struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info);
> +extern struct page *balloon_page_alloc(void);
> +extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
> + struct page *page);
> extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info);
>
> static inline void balloon_devinfo_init(struct balloon_dev_info *balloon)
> @@ -89,6 +92,36 @@ extern int balloon_page_migrate(struct address_space *mapping,
> struct page *page, enum migrate_mode mode);
>
> /*
> + * balloon_page_push - insert a page into a page list.
> + * @head : pointer to list
> + * @page : page to be added
> + *
> + * Caller must ensure the page is private and protect the list.
> + */
> +static inline void balloon_page_push(struct list_head *pages, struct page *page)
> +{
> + list_add(&page->lru, pages);
> +}
> +
> +/*
> + * balloon_page_pop - remove a page from a page list.
> + * @head : pointer to list
> + * @page : page to be added
> + *
> + * Caller must ensure the page is private and protect the list.
> + */
> +static inline struct page *balloon_page_pop(struct list_head *pages)
> +{
> + struct page *page = list_first_entry_or_null(pages, struct page, lru);
> +
> + if (!page)
> + return NULL;
> +
> + list_del(&page->lru);
> + return page;
> +}
> +
> +/*
> * balloon_page_insert - insert a page into the balloon's page list and make
> * the page->private assignment accordingly.
> * @balloon : pointer to balloon device
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index 68d2892..ef858d5 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -11,22 +11,37 @@
> #include <linux/balloon_compaction.h>
>
> /*
> + * balloon_page_alloc - allocates a new page for insertion into the balloon
> + * page list.
> + *
> + * Driver must call it to properly allocate a new enlisted balloon page.
> + * Driver must call balloon_page_enqueue before definitively removing it from
> + * the guest system. This function returns the page address for the recently
> + * allocated page or NULL in the case we fail to allocate a new page this turn.
> + */
> +struct page *balloon_page_alloc(void)
> +{
> + struct page *page = alloc_page(balloon_mapping_gfp_mask() |
> + __GFP_NOMEMALLOC | __GFP_NORETRY);
> + return page;
> +}
> +EXPORT_SYMBOL_GPL(balloon_page_alloc);
> +
> +/*
> * balloon_page_enqueue - allocates a new page and inserts it into the balloon
> * page list.
> * @b_dev_info: balloon device descriptor where we will insert a new page to
> + * @page: new page to enqueue - allocated using balloon_page_alloc.
> *
> - * Driver must call it to properly allocate a new enlisted balloon page
> + * Driver must call it to properly enqueue a new allocated balloon page
> * before definitively removing it from the guest system.
> * This function returns the page address for the recently enqueued page or
> * NULL in the case we fail to allocate a new page this turn.
> */
> -struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info)
> +void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
> + struct page *page)
> {
> unsigned long flags;
> - struct page *page = alloc_page(balloon_mapping_gfp_mask() |
> - __GFP_NOMEMALLOC | __GFP_NORETRY);
> - if (!page)
> - return NULL;
>
> /*
> * Block others from accessing the 'page' when we get around to
> @@ -39,7 +54,6 @@ struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info)
> __count_vm_event(BALLOON_INFLATE);
> spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> unlock_page(page);
> - return page;
> }
> EXPORT_SYMBOL_GPL(balloon_page_enqueue);
>
> --
> MST
^ permalink raw reply
* Re: [PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
From: Sudip Mukherjee @ 2017-12-01 14:10 UTC (permalink / raw)
To: Daniel Vetter
Cc: Teddy Wang, David Airlie, Greg KH, amd-gfx,
Michał Mirosław, dri-devel, Alex Deucher,
virtualization, Christian König
In-Reply-To: <20171201071916.jpl5ucxq4glduho4@phenom.ffwll.local>
On Fri, Dec 01, 2017 at 08:19:16AM +0100, Daniel Vetter wrote:
> On Thu, Nov 30, 2017 at 11:49:53PM +0000, Sudip Mukherjee wrote:
> > Hi Daniel,
> >
> > <snip>
> >
> > > > > >
> > > > > > Greg?
> > > > >
> > > > > Yes, if no one is working to get it out of staging, that means no one
> > > > > cares about it, and it needs to be removed from the tree.
> > > >
> > > > Agreed. I was not working on getting it out of staging as there is no
> > > > place for it to go.
> > > > But, Teddy Wang told me that they have a working drm driver for it, but
> > > > it is not atomic like Daniel was asking for. If it is ok, then I can send
> > > > in a patch to remove the existing sm750 from staging and add the new sm750
> > > > drm driver to staging. And after it is ready, we can go ahead with moving
> > > > it out of staging to drm.
> > >
> > > Please keep the todo item that it needs to be converted to atomic. And
> > > tbh, it's probably faster if you just submit it to dri-devel, assuming you
> > > have time to work on it. For small drivers we tend to be fairly quick in
> > > getting them into good enough shape.
> >
> > I have received the driver from Teddy and pushed it to
> > https://github.com/sudipm-mukherjee/parport/tree/drm_smi for your first
> > look into it. It is not even building with next-20171130 and has lots of
> > build warnings. I will have to do a lot of work on it before I can even
> > submit it to dri-devel.
> >
> > Time will be the problem, as this is not part of my day job.
> >
> > >
> > > Staging is also a major pain for drm subsystem refactorings, I really,
> > > really, really prefer we don't add more than the vbox pain we have
> > > already.
> >
> > I am hoping that after seeing it, you will agree to have it in staging. :)
>
> So I know Greg is willing to take anything into staging, but I'm no fan.
> We refactor and improve drm a lot, with a lot of cross-driver changes
> necessary to move things forward. We can do that since we have a generally
> rather active development community, and we try hard to keep most drivers
> in reasonable good shape and so easy to maintain.
>
> If you know throw a pile of unmaintainable stuff into staging, but without
> working on it, then that's just cost, no benefit to the dri-devel
> community. On top, staging tree is separate from drm trees, so more pain
> to synchronize trees (and we have to do that a few times per release cycle
> or drivers simply stop compiling). Where's the upside of taking this
> driver into staging?
>
> One is users, but ime for soc display drivers usually everything else is
> in worse shape (e.g. even great drivers like the one for qualcom must be
> tested on some vendor tree because critical core bits are missing in
> upstream), so "more users" is not the good reason. And it's clearly not
> "more developers", because no time to clean things up. So what exactly is
> the good reason to put this into staging instead of just waiting until
> someone has the time to clean it up quickly?
Ok, I will not try to give any more reasons now. :)
I will cleanup the basic things I can and then submit it to dri-devel.
Greg - Please keep the SM750 driver in staging for now. I will send
a patch later to add in TODO the git location where the drm driver is
being developed. And when that drm driver is ready, I will send a patch
to remove the sm750fb driver from staging.
--
Regards
Sudip
^ permalink raw reply
* Re: [PATCH v18 05/10] xbitmap: add more operations
From: Matthew Wilcox @ 2017-12-01 14:13 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, quan.xu,
nilal, riel, cornelia.huck, mhocko, linux-kernel, amit.shah,
pbonzini, akpm, mgorman
In-Reply-To: <201712012202.BDE13557.MJFQLtOOHVOFSF@I-love.SAKURA.ne.jp>
On Fri, Dec 01, 2017 at 10:02:01PM +0900, Tetsuo Handa wrote:
> If start == end is legal,
>
> for (; start < end; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
>
> makes this loop do nothing because 10 < 10 is false.
... and this is why we add tests to the test-suite!
^ permalink raw reply
* Re: [PATCH 2/3] tun: free skb in early errors
From: Michael S. Tsirkin @ 2017-12-01 14:36 UTC (permalink / raw)
To: Jason Wang; +Cc: mjrosato, netdev, wexu, linux-kernel, virtualization
In-Reply-To: <a079ad24-cf20-66bc-2f57-9d0df9534f22@redhat.com>
On Fri, Dec 01, 2017 at 03:07:44PM +0800, Jason Wang wrote:
>
>
> On 2017年12月01日 13:54, wexu@redhat.com wrote:
> > From: Wei Xu <wexu@redhat.com>
> >
> > tun_recvmsg() supports accepting skb by msg_control after
> > commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
> > the skb if presented should be freed within the function, otherwise it
> > would be leaked.
> >
> > Signed-off-by: Wei Xu <wexu@redhat.com>
> > Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> > ---
> > drivers/net/tun.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 6a7bde9..5563430 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -2067,14 +2067,17 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
> > {
> > struct tun_file *tfile = container_of(sock, struct tun_file, socket);
> > struct tun_struct *tun = tun_get(tfile);
> > + struct sk_buff *skb = m->msg_control;
> > int ret;
> > - if (!tun)
> > - return -EBADFD;
> > + if (!tun) {
> > + ret = -EBADFD;
> > + goto out_free_skb;
>
> Unfortunately, you can't to there since tun is NULL.
Right, this should just be kfree_skb(skb); return -EBADFD;
>
> > + }
> > if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
> > ret = -EINVAL;
> > - goto out;
> > + goto out_free_skb;
> > }
> > if (flags & MSG_ERRQUEUE) {
> > ret = sock_recv_errqueue(sock->sk, m, total_len,
> > @@ -2087,6 +2090,11 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
> > m->msg_flags |= MSG_TRUNC;
> > ret = flags & MSG_TRUNC ? ret : total_len;
> > }
> > + goto out;
>
> We usually don't use goto in the case of success, and you need deal with the
> case skb != NULL but iov_iter_count(to) == 0 in tun_do_read().
>
> Thanks
I agree, the way to lay this out is:
tun_put(tun);
return ret;
err:
tun_put(tun);
err_tun:
if (skb)
kfree_skb(skb);
return ret;
> > +
> > +out_free_skb:
> > + if (skb)
> > + kfree_skb(skb);
> > out:
> > tun_put(tun);
> > return ret;
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 1/3] vhost: fix skb leak in handle_rx()
From: Michael S. Tsirkin @ 2017-12-01 14:37 UTC (permalink / raw)
To: Jason Wang; +Cc: mjrosato, netdev, wexu, linux-kernel, virtualization
In-Reply-To: <09e75683-4c49-7446-e13e-93b316ed270c@redhat.com>
On Fri, Dec 01, 2017 at 03:11:05PM +0800, Jason Wang wrote:
>
>
> On 2017年12月01日 13:54, wexu@redhat.com wrote:
> > From: Wei Xu <wexu@redhat.com>
> >
> > Matthew found a roughly 40% tcp throughput regression with commit
> > c67df11f(vhost_net: try batch dequing from skb array) as discussed
> > in the following thread:
> > https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
> >
> > Eventually we figured out that it was a skb leak in handle_rx()
> > when sending packets to the VM. This usually happens when a guest
> > can not drain out vq as fast as vhost fills in, afterwards it sets
> > off the traffic jam and leaks skb(s) which occurs as no headcount
> > to send on the vq from vhost side.
> >
> > This can be avoided by making sure we have got enough headcount
> > before actually consuming a skb from the batched rx array while
> > transmitting, which is simply done by moving checking the zero
> > headcount a bit ahead.
> >
> > Signed-off-by: Wei Xu <wexu@redhat.com>
> > Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> > ---
> > drivers/vhost/net.c | 20 ++++++++++----------
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 8d626d7..c7bdeb6 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
> > /* On error, stop handling until the next kick. */
> > if (unlikely(headcount < 0))
> > goto out;
> > - if (nvq->rx_array)
> > - msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> > - /* On overrun, truncate and discard */
> > - if (unlikely(headcount > UIO_MAXIOV)) {
> > - iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> > - err = sock->ops->recvmsg(sock, &msg,
> > - 1, MSG_DONTWAIT | MSG_TRUNC);
> > - pr_debug("Discarded rx packet: len %zd\n", sock_len);
> > - continue;
> > - }
> > /* OK, now we need to know about added descriptors. */
> > if (!headcount) {
> > if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> > @@ -800,6 +790,16 @@ static void handle_rx(struct vhost_net *net)
> > * they refilled. */
> > goto out;
> > }
> > + if (nvq->rx_array)
> > + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> > + /* On overrun, truncate and discard */
> > + if (unlikely(headcount > UIO_MAXIOV)) {
> > + iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> > + err = sock->ops->recvmsg(sock, &msg,
> > + 1, MSG_DONTWAIT | MSG_TRUNC);
> > + pr_debug("Discarded rx packet: len %zd\n", sock_len);
> > + continue;
> > + }
> > /* We don't need to be notified again. */
> > iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
> > fixup = msg.msg_iter;
>
> I suggest to reorder this patch to 3/3.
>
> Thanks
Why? This doesn't cause any new leaks, does it?
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
From: Emil Velikov @ 2017-12-01 14:40 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: Teddy Wang, David Airlie, Greg KH, amd-gfx mailing list,
Michał Mirosław, ML dri-devel, Daniel Vetter,
Alex Deucher, open list:VIRTIO GPU DRIVER, Christian König
In-Reply-To: <20171130234953.GA5542@sudip-laptop>
On 30 November 2017 at 23:49, Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
> Hi Daniel,
>
> On Wed, Nov 29, 2017 at 10:56:34AM +0100, Daniel Vetter wrote:
>> On Tue, Nov 28, 2017 at 12:30:30PM +0000, Sudip Mukherjee wrote:
>> > On Tue, Nov 28, 2017 at 12:32:38PM +0100, Greg KH wrote:
>> > > On Tue, Nov 28, 2017 at 11:22:17AM +0100, Daniel Vetter wrote:
>> > > > On Mon, Nov 27, 2017 at 08:52:19PM +0000, Sudip Mukherjee wrote:
>> > > > > On Mon, Nov 27, 2017 at 11:27:59AM +0100, Daniel Vetter wrote:
>> > > > > > On Fri, Nov 24, 2017 at 06:53:31PM +0100, Michał Mirosław wrote:
>> > > > > > > Almost all drivers using remove_conflicting_framebuffers() wrap it with
>> > > > > > > the same code. Extract common part from PCI drivers into separate
>> > > > > > > remove_conflicting_pci_framebuffers().
>> > > > > > >
>
> <snip>
>
>> > > >
>> > > > Greg?
>> > >
>> > > Yes, if no one is working to get it out of staging, that means no one
>> > > cares about it, and it needs to be removed from the tree.
>> >
>> > Agreed. I was not working on getting it out of staging as there is no
>> > place for it to go.
>> > But, Teddy Wang told me that they have a working drm driver for it, but
>> > it is not atomic like Daniel was asking for. If it is ok, then I can send
>> > in a patch to remove the existing sm750 from staging and add the new sm750
>> > drm driver to staging. And after it is ready, we can go ahead with moving
>> > it out of staging to drm.
>>
>> Please keep the todo item that it needs to be converted to atomic. And
>> tbh, it's probably faster if you just submit it to dri-devel, assuming you
>> have time to work on it. For small drivers we tend to be fairly quick in
>> getting them into good enough shape.
>
> I have received the driver from Teddy and pushed it to
> https://github.com/sudipm-mukherjee/parport/tree/drm_smi for your first
> look into it. It is not even building with next-20171130 and has lots of
> build warnings. I will have to do a lot of work on it before I can even
> submit it to dri-devel.
>
A crazy idea, mostly towards Tedd and Sudip:
Start small and build gradually. An example split for separate patch series:
- one HW, basic setup + atomic KMS
- add second HW
- more KMS features
- fancy memory management
- 2D/3D/other acceleration
The driver as seen above tries to do all of the above (almost, it's not atomic)
at once - 40k loc.
Someone familiar with the code can quickly split it up and while doing
so, feed it through checkpatch.
Current code is _very_ far from kernel coding style, plus the
copyright blurp is very disturbing:
* All rights are reserved. Reproduction or in part is prohibited
* without the written consent of the copyright owner.
HTH
Emil
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: Memory corruption in powerpc guests with virtio_balloon (was Re: [PATCH v3] virtio_balloon: fix deadlock on OOM)
From: Michael S. Tsirkin @ 2017-12-01 14:42 UTC (permalink / raw)
To: Michael Ellerman
Cc: Michal Hocko, Tetsuo Handa, linux-kernel, virtualization,
linux-mm, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <87o9nid3zn.fsf@concordia.ellerman.id.au>
On Fri, Dec 01, 2017 at 11:31:08PM +1100, Michael Ellerman wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> > fill_balloon doing memory allocations under balloon_lock
> > can cause a deadlock when leak_balloon is called from
> > virtballoon_oom_notify and tries to take same lock.
> >
> > To fix, split page allocation and enqueue and do allocations outside the lock.
> >
> > Here's a detailed analysis of the deadlock by Tetsuo Handa:
> >
> > In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
> > serialize against fill_balloon(). But in fill_balloon(),
> > alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
> > called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE]
> > implies __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, despite __GFP_NORETRY
> > is specified, this allocation attempt might indirectly depend on somebody
> > else's __GFP_DIRECT_RECLAIM memory allocation. And such indirect
> > __GFP_DIRECT_RECLAIM memory allocation might call leak_balloon() via
> > virtballoon_oom_notify() via blocking_notifier_call_chain() callback via
> > out_of_memory() when it reached __alloc_pages_may_oom() and held oom_lock
> > mutex. Since vb->balloon_lock mutex is already held by fill_balloon(), it
> > will cause OOM lockup.
> >
> > Thread1 Thread2
> > fill_balloon()
> > takes a balloon_lock
> > balloon_page_enqueue()
> > alloc_page(GFP_HIGHUSER_MOVABLE)
> > direct reclaim (__GFP_FS context) takes a fs lock
> > waits for that fs lock alloc_page(GFP_NOFS)
> > __alloc_pages_may_oom()
> > takes the oom_lock
> > out_of_memory()
> > blocking_notifier_call_chain()
> > leak_balloon()
> > tries to take that balloon_lock and deadlocks
> >
> > Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Wei Wang <wei.w.wang@intel.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > drivers/virtio/virtio_balloon.c | 24 +++++++++++++++++++-----
> > include/linux/balloon_compaction.h | 35 ++++++++++++++++++++++++++++++++++-
> > mm/balloon_compaction.c | 28 +++++++++++++++++++++-------
> > 3 files changed, 74 insertions(+), 13 deletions(-)
>
>
> Somehow this commit seems to be killing powerpc guests.
>
> The symptom is that the first page (64K) of the guests memory gets over
> written with zeroes, which is where our interrupt handlers are, so the
> system rapidly locks up due to illegal instructions in the illegal
> instruction handler.
>
> There seems to be some element of a race, because it doesn't always
> crash. Sometimes I can boot to a shell, but not often. When it does
> happen it's fairly late in boot, but before I get to a shell.
>
> I had a few bisects go off into the weeds due to the intermittent
> nature. But once I realised that I changed my script to boot 5 times
> before declaring a kernel good, and that bisected straight here.
>
> I can also revert this commit on v4.15-rc1 and everything's fine again -
> I got through ~250 boots with that kernel.
>
> So I'm pretty sure this commit is triggering/exposing/causing the bug.
>
> The other data point is that the page that's overwritten is mapped read
> only in the guest kernel. So either the guest kernel is writing to it in
> real mode (MMU off), or the hypervisor/DMA is doing it.
>
> I haven't isolated if it's host kernel/qemu version dependent, at the
> moment I'm just using distro packaged versions of both.
>
> Anyway I'll try and dig further into it on Monday, but I thought I'd let
> you know in case this is a known bug with a fix in the pipeline, or
> rings any bells or whatever.
>
> cheers
Thanks for the report!
A fix was just posted:
virtio_balloon: fix increment of vb->num_pfns in fill_balloon()
Would appreciate testing.
>
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index f0b3a0b..7960746 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -143,16 +143,17 @@ static void set_page_pfns(struct virtio_balloon *vb,
> >
> > static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> > {
> > - struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> > unsigned num_allocated_pages;
> > + unsigned num_pfns;
> > + struct page *page;
> > + LIST_HEAD(pages);
> >
> > /* We can only do one array worth at a time. */
> > num = min(num, ARRAY_SIZE(vb->pfns));
> >
> > - mutex_lock(&vb->balloon_lock);
> > - for (vb->num_pfns = 0; vb->num_pfns < num;
> > - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> > - struct page *page = balloon_page_enqueue(vb_dev_info);
> > + for (num_pfns = 0; num_pfns < num;
> > + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> > + struct page *page = balloon_page_alloc();
> >
> > if (!page) {
> > dev_info_ratelimited(&vb->vdev->dev,
> > @@ -162,6 +163,19 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> > msleep(200);
> > break;
> > }
> > +
> > + balloon_page_push(&pages, page);
> > + }
> > +
> > + mutex_lock(&vb->balloon_lock);
> > +
> > + vb->num_pfns = 0;
> > +
> > + while ((page = balloon_page_pop(&pages))) {
> > + balloon_page_enqueue(&vb->vb_dev_info, page);
> > +
> > + vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > +
> > set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > if (!virtio_has_feature(vb->vdev,
> > diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> > index fbbe6da..c4c8df9 100644
> > --- a/include/linux/balloon_compaction.h
> > +++ b/include/linux/balloon_compaction.h
> > @@ -50,6 +50,7 @@
> > #include <linux/gfp.h>
> > #include <linux/err.h>
> > #include <linux/fs.h>
> > +#include <linux/list.h>
> >
> > /*
> > * Balloon device information descriptor.
> > @@ -67,7 +68,9 @@ struct balloon_dev_info {
> > struct inode *inode;
> > };
> >
> > -extern struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info);
> > +extern struct page *balloon_page_alloc(void);
> > +extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
> > + struct page *page);
> > extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info);
> >
> > static inline void balloon_devinfo_init(struct balloon_dev_info *balloon)
> > @@ -89,6 +92,36 @@ extern int balloon_page_migrate(struct address_space *mapping,
> > struct page *page, enum migrate_mode mode);
> >
> > /*
> > + * balloon_page_push - insert a page into a page list.
> > + * @head : pointer to list
> > + * @page : page to be added
> > + *
> > + * Caller must ensure the page is private and protect the list.
> > + */
> > +static inline void balloon_page_push(struct list_head *pages, struct page *page)
> > +{
> > + list_add(&page->lru, pages);
> > +}
> > +
> > +/*
> > + * balloon_page_pop - remove a page from a page list.
> > + * @head : pointer to list
> > + * @page : page to be added
> > + *
> > + * Caller must ensure the page is private and protect the list.
> > + */
> > +static inline struct page *balloon_page_pop(struct list_head *pages)
> > +{
> > + struct page *page = list_first_entry_or_null(pages, struct page, lru);
> > +
> > + if (!page)
> > + return NULL;
> > +
> > + list_del(&page->lru);
> > + return page;
> > +}
> > +
> > +/*
> > * balloon_page_insert - insert a page into the balloon's page list and make
> > * the page->private assignment accordingly.
> > * @balloon : pointer to balloon device
> > diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> > index 68d2892..ef858d5 100644
> > --- a/mm/balloon_compaction.c
> > +++ b/mm/balloon_compaction.c
> > @@ -11,22 +11,37 @@
> > #include <linux/balloon_compaction.h>
> >
> > /*
> > + * balloon_page_alloc - allocates a new page for insertion into the balloon
> > + * page list.
> > + *
> > + * Driver must call it to properly allocate a new enlisted balloon page.
> > + * Driver must call balloon_page_enqueue before definitively removing it from
> > + * the guest system. This function returns the page address for the recently
> > + * allocated page or NULL in the case we fail to allocate a new page this turn.
> > + */
> > +struct page *balloon_page_alloc(void)
> > +{
> > + struct page *page = alloc_page(balloon_mapping_gfp_mask() |
> > + __GFP_NOMEMALLOC | __GFP_NORETRY);
> > + return page;
> > +}
> > +EXPORT_SYMBOL_GPL(balloon_page_alloc);
> > +
> > +/*
> > * balloon_page_enqueue - allocates a new page and inserts it into the balloon
> > * page list.
> > * @b_dev_info: balloon device descriptor where we will insert a new page to
> > + * @page: new page to enqueue - allocated using balloon_page_alloc.
> > *
> > - * Driver must call it to properly allocate a new enlisted balloon page
> > + * Driver must call it to properly enqueue a new allocated balloon page
> > * before definitively removing it from the guest system.
> > * This function returns the page address for the recently enqueued page or
> > * NULL in the case we fail to allocate a new page this turn.
> > */
> > -struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info)
> > +void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
> > + struct page *page)
> > {
> > unsigned long flags;
> > - struct page *page = alloc_page(balloon_mapping_gfp_mask() |
> > - __GFP_NOMEMALLOC | __GFP_NORETRY);
> > - if (!page)
> > - return NULL;
> >
> > /*
> > * Block others from accessing the 'page' when we get around to
> > @@ -39,7 +54,6 @@ struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info)
> > __count_vm_event(BALLOON_INFLATE);
> > spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> > unlock_page(page);
> > - return page;
> > }
> > EXPORT_SYMBOL_GPL(balloon_page_enqueue);
> >
> > --
> > MST
^ permalink raw reply
* Re: [PATCH net,stable v4 0/3] vhost: fix a few skb leaks
From: Michael S. Tsirkin @ 2017-12-01 14:47 UTC (permalink / raw)
To: wexu; +Cc: mjrosato, netdev, linux-kernel, virtualization
In-Reply-To: <1512123038-15773-1-git-send-email-wexu@redhat.com>
On Fri, Dec 01, 2017 at 05:10:35AM -0500, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Matthew found a roughly 40% tcp throughput regression with commit
> c67df11f(vhost_net: try batch dequing from skb array) as discussed
> in the following thread:
> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
Series
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> v4:
> - fix zero iov iterator count in tap/tap_do_read()(Jason)
> - don't put tun in case of EBADFD(Jason)
> - Replace msg->msg_control with new 'skb' when calling tun/tap_do_read()
>
> v3:
> - move freeing skb from vhost to tun/tap recvmsg() to not
> confuse the callers.
>
> v2:
> - add Matthew as the reporter, thanks matthew.
> - moving zero headcount check ahead instead of defer consuming skb
> due to jason and mst's comment.
> - add freeing skb in favor of recvmsg() fails.
>
> Wei Xu (3):
> vhost: fix skb leak in handle_rx()
> tun: free skb in early errors
> tap: free skb if flags error
>
> drivers/net/tap.c | 14 ++++++++++----
> drivers/net/tun.c | 24 ++++++++++++++++++------
> drivers/vhost/net.c | 20 ++++++++++----------
> 3 files changed, 38 insertions(+), 20 deletions(-)
>
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH 1/3] vhost: fix skb leak in handle_rx()
From: Michael S. Tsirkin @ 2017-12-01 14:48 UTC (permalink / raw)
To: wexu; +Cc: mjrosato, netdev, linux-kernel, virtualization
In-Reply-To: <1512123038-15773-2-git-send-email-wexu@redhat.com>
On Fri, Dec 01, 2017 at 05:10:36AM -0500, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Matthew found a roughly 40% tcp throughput regression with commit
> c67df11f(vhost_net: try batch dequing from skb array) as discussed
> in the following thread:
> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
>
> Eventually we figured out that it was a skb leak in handle_rx()
> when sending packets to the VM. This usually happens when a guest
> can not drain out vq as fast as vhost fills in, afterwards it sets
> off the traffic jam and leaks skb(s) which occurs as no headcount
> to send on the vq from vhost side.
>
> This can be avoided by making sure we have got enough headcount
> before actually consuming a skb from the batched rx array while
> transmitting, which is simply done by moving checking the zero
> headcount a bit ahead.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/net.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 8d626d7..c7bdeb6 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
> /* On error, stop handling until the next kick. */
> if (unlikely(headcount < 0))
> goto out;
> - if (nvq->rx_array)
> - msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> - /* On overrun, truncate and discard */
> - if (unlikely(headcount > UIO_MAXIOV)) {
> - iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> - err = sock->ops->recvmsg(sock, &msg,
> - 1, MSG_DONTWAIT | MSG_TRUNC);
> - pr_debug("Discarded rx packet: len %zd\n", sock_len);
> - continue;
> - }
> /* OK, now we need to know about added descriptors. */
> if (!headcount) {
> if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> @@ -800,6 +790,16 @@ static void handle_rx(struct vhost_net *net)
> * they refilled. */
> goto out;
> }
> + if (nvq->rx_array)
> + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> + /* On overrun, truncate and discard */
> + if (unlikely(headcount > UIO_MAXIOV)) {
> + iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> + err = sock->ops->recvmsg(sock, &msg,
> + 1, MSG_DONTWAIT | MSG_TRUNC);
> + pr_debug("Discarded rx packet: len %zd\n", sock_len);
> + continue;
> + }
> /* We don't need to be notified again. */
> iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
> fixup = msg.msg_iter;
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH 2/3] tun: free skb in early errors
From: Michael S. Tsirkin @ 2017-12-01 14:48 UTC (permalink / raw)
To: wexu; +Cc: mjrosato, netdev, linux-kernel, virtualization
In-Reply-To: <1512123038-15773-3-git-send-email-wexu@redhat.com>
On Fri, Dec 01, 2017 at 05:10:37AM -0500, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> tun_recvmsg() supports accepting skb by msg_control after
> commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
> the skb if presented should be freed no matter how far it can go
> along, otherwise it would be leaked.
>
> This patch fixes several missed cases.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/tun.c | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 9574900..4f4a842 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1952,8 +1952,11 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
>
> tun_debug(KERN_INFO, tun, "tun_do_read\n");
>
> - if (!iov_iter_count(to))
> + if (!iov_iter_count(to)) {
> + if (skb)
> + kfree_skb(skb);
> return 0;
> + }
>
> if (!skb) {
> /* Read frames from ring */
> @@ -2069,22 +2072,24 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
> {
> struct tun_file *tfile = container_of(sock, struct tun_file, socket);
> struct tun_struct *tun = tun_get(tfile);
> + struct sk_buff *skb = m->msg_control;
> int ret;
>
> - if (!tun)
> - return -EBADFD;
> + if (!tun) {
> + ret = -EBADFD;
> + goto out_free_skb;
> + }
>
> if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
> ret = -EINVAL;
> - goto out;
> + goto out_put_tun;
> }
> if (flags & MSG_ERRQUEUE) {
> ret = sock_recv_errqueue(sock->sk, m, total_len,
> SOL_PACKET, TUN_TX_TIMESTAMP);
> goto out;
> }
> - ret = tun_do_read(tun, tfile, &m->msg_iter, flags & MSG_DONTWAIT,
> - m->msg_control);
> + ret = tun_do_read(tun, tfile, &m->msg_iter, flags & MSG_DONTWAIT, skb);
> if (ret > (ssize_t)total_len) {
> m->msg_flags |= MSG_TRUNC;
> ret = flags & MSG_TRUNC ? ret : total_len;
> @@ -2092,6 +2097,13 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
> out:
> tun_put(tun);
> return ret;
> +
> +out_put_tun:
> + tun_put(tun);
> +out_free_skb:
> + if (skb)
> + kfree_skb(skb);
> + return ret;
> }
>
> static int tun_peek_len(struct socket *sock)
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH 3/3] tap: free skb if flags error
From: Michael S. Tsirkin @ 2017-12-01 14:48 UTC (permalink / raw)
To: wexu; +Cc: mjrosato, netdev, linux-kernel, virtualization
In-Reply-To: <1512123038-15773-4-git-send-email-wexu@redhat.com>
On Fri, Dec 01, 2017 at 05:10:38AM -0500, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> tap_recvmsg() supports accepting skb by msg_control after
> commit 3b4ba04acca8 ("tap: support receiving skb from msg_control"),
> the skb if presented should be freed within the function, otherwise
> it would be leaked.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/tap.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index e9489b8..0a886fda 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -829,8 +829,11 @@ static ssize_t tap_do_read(struct tap_queue *q,
> DEFINE_WAIT(wait);
> ssize_t ret = 0;
>
> - if (!iov_iter_count(to))
> + if (!iov_iter_count(to)) {
> + if (skb)
> + kfree_skb(skb);
> return 0;
> + }
>
> if (skb)
> goto put;
> @@ -1154,11 +1157,14 @@ static int tap_recvmsg(struct socket *sock, struct msghdr *m,
> size_t total_len, int flags)
> {
> struct tap_queue *q = container_of(sock, struct tap_queue, sock);
> + struct sk_buff *skb = m->msg_control;
> int ret;
> - if (flags & ~(MSG_DONTWAIT|MSG_TRUNC))
> + if (flags & ~(MSG_DONTWAIT|MSG_TRUNC)) {
> + if (skb)
> + kfree_skb(skb);
> return -EINVAL;
> - ret = tap_do_read(q, &m->msg_iter, flags & MSG_DONTWAIT,
> - m->msg_control);
> + }
> + ret = tap_do_read(q, &m->msg_iter, flags & MSG_DONTWAIT, skb);
> if (ret > total_len) {
> m->msg_flags |= MSG_TRUNC;
> ret = flags & MSG_TRUNC ? ret : total_len;
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH net,stable v4 0/3] vhost: fix a few skb leaks
From: Matthew Rosato @ 2017-12-01 14:54 UTC (permalink / raw)
To: Michael S. Tsirkin, wexu; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20171201164731-mutt-send-email-mst@kernel.org>
On 12/01/2017 09:47 AM, Michael S. Tsirkin wrote:
> On Fri, Dec 01, 2017 at 05:10:35AM -0500, wexu@redhat.com wrote:
>> From: Wei Xu <wexu@redhat.com>
>>
>> Matthew found a roughly 40% tcp throughput regression with commit
>> c67df11f(vhost_net: try batch dequing from skb array) as discussed
>> in the following thread:
>> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
>
> Series
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
Tested this series on a z13 (s390x) on top of net-next using 4GB/4vcpu
guests. Verified that both the reported TCP throughput regression and
memory leak are resolved.
net-next: 19.83 Gb/s
net-next+: 35.02 Gb/s
Thanks all!
Matt
>
>
>> v4:
>> - fix zero iov iterator count in tap/tap_do_read()(Jason)
>> - don't put tun in case of EBADFD(Jason)
>> - Replace msg->msg_control with new 'skb' when calling tun/tap_do_read()
>>
>> v3:
>> - move freeing skb from vhost to tun/tap recvmsg() to not
>> confuse the callers.
>>
>> v2:
>> - add Matthew as the reporter, thanks matthew.
>> - moving zero headcount check ahead instead of defer consuming skb
>> due to jason and mst's comment.
>> - add freeing skb in favor of recvmsg() fails.
>>
>> Wei Xu (3):
>> vhost: fix skb leak in handle_rx()
>> tun: free skb in early errors
>> tap: free skb if flags error
>>
>> drivers/net/tap.c | 14 ++++++++++----
>> drivers/net/tun.c | 24 ++++++++++++++++++------
>> drivers/vhost/net.c | 20 ++++++++++----------
>> 3 files changed, 38 insertions(+), 20 deletions(-)
>>
>> --
>> 1.8.3.1
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox