Re: BUG: KASAN: use-after-free in free_old_xmit_skbs

[Michael S. Tsirkin]

On Thu, Jun 22, 2017 at 08:15:58AM +0200, jean-philippe menil wrote:
> 2017-06-06 1:52 GMT+02:00 Michael S. Tsirkin :
> 
> On Mon, Jun 05, 2017 at 05:08:25AM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jun 05, 2017 at 12:48:53AM +0200, Jean-Philippe Menil wrote:
> > > Hi,
> > >
> > > while playing with xdp and ebpf, i'm hitting the following:
> > >
> > > [  309.993136]
> > > ==
> > > [  309.994735] BUG: KASAN: use-after-free in
> > > free_old_xmit_skbs.isra.29+0x2b7/0x2e0 [virtio_net]
> > > [  309.998396] Read of size 8 at addr 88006aa64220 by task 
> sshd/323
> > > [  310.000650]
> > > [  310.002305] CPU: 1 PID: 323 Comm: sshd Not tainted 4.12.0-rc3+ #2
> > > [  310.004018] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS
> > > 1.10.2-20170228_101828-anatol 04/01/2014

...

> >
> > Since commit 680557cf79f82623e2c4fd42733077d60a843513
> >     virtio_net: rework mergeable buffer handling
> >
> > we no longer must do the resets, we now have enough space
> > to store a bit saying whether a buffer is xdp one or not.
> >
> > And that's probably a cleaner way to fix these issues than
> > try to find and fix the race condition.
> >
> > John?
> >
> > --
> > MST
> 
> 
> I think I see the source of the race. virtio net calls
> netif_device_detach and assumes no packets will be sent after
> this point. However, all it does is stop all queues so
> no new packets will be transmitted.
> 
> Try locking with HARD_TX_LOCK?
>
> 
> --
> MST
> 
> 
> Hi Michael,
> 
> from what i see, the race appear when we hit virtnet_reset in virtnet_xdp_set.
> virtnet_reset
>   _remove_vq_common
>     virtnet_del_vqs
>   virtnet_free_queues
>     kfree(vi->sq)
> when the xdp program (with two instances of the program to trigger it faster)
> is added or removed.
> 
> It's easily repeatable, with 2 cpus and 4 queues on the qemu command line,
> running the xdp_ttl tool from Jesper.
> 
> For now, i'm able to continue my qualification, testing if xdp_qp is not null,
> but do not seem to be a sustainable trick.
> if (xdp_qp && vi->xdp_queues_pairs != xdp_qp)
> 
> Maybe it will be more clear to you with theses informations.
> 
> Best regards.
> 
> Jean-Philippe


I'm pretty clear about the issue here, I was trying to figure out a fix.
Jason, any thoughts?


-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-dev] Re: [PATCH v11 6/6] virtio-balloon: VIRTIO_BALLOON_F_CMD_VQ

[Wei Wang]

On 06/21/2017 08:28 PM, Michael S. Tsirkin wrote:

On Wed, Jun 21, 2017 at 11:28:00AM +0800, Wei Wang wrote:

On 06/21/2017 12:18 AM, Michael S. Tsirkin wrote:

On Fri, Jun 09, 2017 at 06:41:41PM +0800, Wei Wang wrote:

-   if (!virtqueue_indirect_desc_table_add(vq, desc, num)) {
+   if (!virtqueue_indirect_desc_table_add(vq, desc, *num)) {
virtqueue_kick(vq);
-   wait_event(vb->acked, virtqueue_get_buf(vq, ));
-   vb->balloon_page_chunk.chunk_num = 0;
+   if (busy_wait)
+   while (!virtqueue_get_buf(vq, ) &&
+  !virtqueue_is_broken(vq))
+   cpu_relax();
+   else
+   wait_event(vb->acked, virtqueue_get_buf(vq, ));

This is something I didn't previously notice.
As you always keep a single buffer in flight, you do not
really need indirect at all. Just add all descriptors
in the ring directly, then kick.

E.g.
virtqueue_add_first
virtqueue_add_next
virtqueue_add_last

?

You also want a flag to avoid allocations but there's no need to do it
per descriptor, set it on vq.


Without using the indirect table, I'm thinking about changing to use
the standard sg (i.e. struct scatterlist), instead of vring_desc, so that
we don't need to modify or add any new functions of virtqueue_add().

In this case, we will kmalloc an array of sgs in probe(), and we can add
the sgs one by one to the vq, which won't trigger the allocation of an
indirect table inside virtqueue_add(), and then kick when all are added.

Best,
Wei

And allocate headers too? This can work. API extensions aren't
necessarily a bad idea though. The API I suggest above is preferable
for the simple reason that it can work without INDIRECT flag
support in hypervisor.


OK, probably we don't need to add a desc to the vq - we can just use
the vq's desc, like this:

int virtqueue_add_first(struct virtqueue *_vq,
 uint64_t addr,
 uint32_t len,
 bool in,
 unsigned int *idx) {

...
   uint16_t desc_flags = in ? VRING_DESC_F_NEXT | VRING_DESC_F_WRITE :
 VRING_DESC_F_NEXT;

vq->vring.desc[vq->free_head].addr = addr;
vq->vring.desc[vq->free_head].len = len;
vq->vring.desc[vq->free_head].flags = cpu_to_virtio16(_vq->vdev, 
flags);

/* return to the caller the desc id */
*idx = vq->free_head;
...
}

int virtqueue_add_next(struct virtqueue *_vq,
 uint64_t addr,
 uint32_t len,
 bool in,
 bool end,
 unsigned int *idx) {
...
vq->vring.desc[*idx].next = vq->free_head;
vq->vring.desc[vq->free_head].addr = addr;
...
if (end)
remove the VRING_DESC_F_NEXT flag
}


What do you think? We can also combine the two functions into one.



Best,
Wei
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 11/11] drm: remove unused and redundant callbacks

[Boris Brezillon]

On Thu, 22 Jun 2017 08:37:55 +0200
Daniel Vetter  wrote:

> On Thu, Jun 22, 2017 at 12:34:36AM +0800, kbuild test robot wrote:
> > Hi Peter,
> > 
> > [auto build test ERROR on drm/drm-next]
> > [also build test ERROR on next-20170621]
> > [cannot apply to v4.12-rc6]
> > [if your patch is applied to the wrong git tree, please drop us a note to 
> > help improve the system]
> > 
> > url:
> > https://github.com/0day-ci/linux/commits/Peter-Rosin/improve-the-fb_setcmap-helper/20170621-205441
> > base:   git://people.freedesktop.org/~airlied/linux.git drm-next
> > config: arm-allmodconfig (attached as .config)
> > compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
> > reproduce:
> > wget 
> > https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
> > ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # save the attached .config to linux build tree
> > make.cross ARCH=arm 
> > 
> > All errors (new ones prefixed by >>):
> >   
> > >> drivers/gpu//drm/armada/armada_fbdev.c:121:2: error: unknown field 
> > >> 'gamma_set' specified in initializer  
> >  .gamma_set = armada_drm_crtc_gamma_set,  
> 
> Looks like you've missed at least the armada driver in your conversion?

Already fixed in v2 ;-).
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 11/11] drm: remove unused and redundant callbacks

[Daniel Vetter]

On Thu, Jun 22, 2017 at 12:34:36AM +0800, kbuild test robot wrote:
> Hi Peter,
> 
> [auto build test ERROR on drm/drm-next]
> [also build test ERROR on next-20170621]
> [cannot apply to v4.12-rc6]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Peter-Rosin/improve-the-fb_setcmap-helper/20170621-205441
> base:   git://people.freedesktop.org/~airlied/linux.git drm-next
> config: arm-allmodconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget 
> https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=arm 
> 
> All errors (new ones prefixed by >>):
> 
> >> drivers/gpu//drm/armada/armada_fbdev.c:121:2: error: unknown field 
> >> 'gamma_set' specified in initializer
>  .gamma_set = armada_drm_crtc_gamma_set,

Looks like you've missed at least the armada driver in your conversion?
-Daniel

>  ^
> >> drivers/gpu//drm/armada/armada_fbdev.c:121:15: error: initialization from 
> >> incompatible pointer type [-Werror=incompatible-pointer-types]
>  .gamma_set = armada_drm_crtc_gamma_set,
>   ^
>drivers/gpu//drm/armada/armada_fbdev.c:121:15: note: (near initialization 
> for 'armada_fb_helper_funcs.fb_probe')
> >> drivers/gpu//drm/armada/armada_fbdev.c:122:2: error: unknown field 
> >> 'gamma_get' specified in initializer
>  .gamma_get = armada_drm_crtc_gamma_get,
>  ^
>drivers/gpu//drm/armada/armada_fbdev.c:122:15: error: initialization from 
> incompatible pointer type [-Werror=incompatible-pointer-types]
>  .gamma_get = armada_drm_crtc_gamma_get,
>   ^
>drivers/gpu//drm/armada/armada_fbdev.c:122:15: note: (near initialization 
> for 'armada_fb_helper_funcs.initial_config')
>cc1: some warnings being treated as errors
> 
> vim +/gamma_set +121 drivers/gpu//drm/armada/armada_fbdev.c
> 
> 96f60e37 Russell King   2012-08-15  115   ret = 1;
> 96f60e37 Russell King   2012-08-15  116   }
> 96f60e37 Russell King   2012-08-15  117   return ret;
> 96f60e37 Russell King   2012-08-15  118  }
> 96f60e37 Russell King   2012-08-15  119  
> 3a493879 Thierry Reding 2014-06-27  120  static const struct 
> drm_fb_helper_funcs armada_fb_helper_funcs = {
> 96f60e37 Russell King   2012-08-15 @121   .gamma_set  = 
> armada_drm_crtc_gamma_set,
> 96f60e37 Russell King   2012-08-15 @122   .gamma_get  = 
> armada_drm_crtc_gamma_get,
> 96f60e37 Russell King   2012-08-15  123   .fb_probe   = 
> armada_fb_probe,
> 96f60e37 Russell King   2012-08-15  124  };
> 96f60e37 Russell King   2012-08-15  125  
> 
> :: The code at line 121 was first introduced by commit
> :: 96f60e37dc66091bde8d5de136ff6fda09f2d799 DRM: Armada: Add Armada DRM 
> driver
> 
> :: TO: Russell King 
> :: CC: Russell King 
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation


> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
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

Re: [PATCH 01/11] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set

[Daniel Vetter]

On Wed, Jun 21, 2017 at 11:40:52AM +0200, Peter Rosin wrote:
> On 2017-06-21 09:38, Daniel Vetter wrote:
> > On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote:
> >> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
> >> totally obsolete.
> >>
> >> I think the gamma_store can end up invalid on error. But the way I read
> >> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should
> >> this pesky legacy fbdev stuff be any better?
> >>
> >> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However,
> >> it saves it to the gamma_store which should already be up to date with what
> >> .gamma_get would return and is thus a nop. So, zap it.
> > 
> > Removing drm_fb_helper_save_lut_atomic should be a separate patch I
> > think.
> 
> Then 3 patches would be needed, first some hybrid thing that does it the
> old way, but also stores the lut in .gamma_store, then the split-out that
> removes drm_fb_helper_save_lut_atomic, then whatever is needed to get
> to the desired code. I can certainly do that, but do you want me to?

Explain that in the commit message and it's fine.

> > It's a pre-existing bug, but should we also try to restore the fbdev lut
> > in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug,
> > but might be relevant for your use-case. Just try to run both an fbdev
> > application and some kms-native thing, and then SIGKILL the native kms
> > app.
> > 
> > But since pre-existing not really required, and probably too much effort.
> 
> Good thing too, because I don't really know my way around this code...

Btw I cc'ed you on one of my patches in the fbdev locking series, we might
need to do the same legacy vs. atomic split for the new lut code as I did
for dpms. The rule with atomic is that you can't do multiple commits under
drm_modeset_lock_all, you either have to do one overall atomic commit
(preferred) or drop locks again. This matters for LUT since
you're updating the LUT on all CRTCs, which when using the gamma_set
atomic helper would be multiple commits :-/

Using the dpms patch as template it shouldn't be too hard to address that
for your patch here too.
-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