* RE: [PATCH 2/6] mdev: introduce device specific ops
From: Parav Pandit @ 2019-09-23 22:59 UTC (permalink / raw)
To: Jason Wang, kvm@vger.kernel.org, linux-s390@vger.kernel.org,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org,
intel-gvt-dev@lists.freedesktop.org, kwankhede@nvidia.com,
alex.williamson@redhat.com, mst@redhat.com, tiwei.bie@intel.com
Cc: virtualization@lists.linux-foundation.org, netdev@vger.kernel.org,
cohuck@redhat.com, maxime.coquelin@redhat.com,
cunming.liang@intel.com, zhihong.wang@intel.com,
rob.miller@broadcom.com, xiao.w.wang@intel.com,
haotian.wang@sifive.com, zhenyuw@linux.intel.com,
zhi.a.wang@intel.com, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
airlied@linux.ie, daniel@ffwll.ch, farman@linux.ibm.com,
pasic@linux.ibm.com, sebott@linux.ibm.com, oberpar@linux.ibm.com,
heiko.carstens@de.ibm.com, gor@linux.ibm.com,
borntraeger@de.ibm.com, akrowiak@linux.ibm.com,
freude@linux.ibm.com, lingshan.zhu@intel.com, Ido Shamay,
eperezma@redhat.com, lulu@redhat.com
In-Reply-To: <20190923130331.29324-3-jasowang@redhat.com>
> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, September 23, 2019 8:03 AM
> To: kvm@vger.kernel.org; linux-s390@vger.kernel.org; linux-
> kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org;
> kwankhede@nvidia.com; alex.williamson@redhat.com; mst@redhat.com;
> tiwei.bie@intel.com
> Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org;
> cohuck@redhat.com; maxime.coquelin@redhat.com;
> cunming.liang@intel.com; zhihong.wang@intel.com;
> rob.miller@broadcom.com; xiao.w.wang@intel.com;
> haotian.wang@sifive.com; zhenyuw@linux.intel.com; zhi.a.wang@intel.com;
> jani.nikula@linux.intel.com; joonas.lahtinen@linux.intel.com;
> rodrigo.vivi@intel.com; airlied@linux.ie; daniel@ffwll.ch;
> farman@linux.ibm.com; pasic@linux.ibm.com; sebott@linux.ibm.com;
> oberpar@linux.ibm.com; heiko.carstens@de.ibm.com; gor@linux.ibm.com;
> borntraeger@de.ibm.com; akrowiak@linux.ibm.com; freude@linux.ibm.com;
> lingshan.zhu@intel.com; Ido Shamay <idos@mellanox.com>;
> eperezma@redhat.com; lulu@redhat.com; Parav Pandit
> <parav@mellanox.com>; Jason Wang <jasowang@redhat.com>
> Subject: [PATCH 2/6] mdev: introduce device specific ops
>
> Currently, except for the create and remove. The rest of mdev_parent_ops is
> designed for vfio-mdev driver only and may not help for kernel mdev driver.
> Follow the class id support by previous patch, this patch introduces device
> specific ops pointer inside parent ops which points to device specific ops (e.g
> vfio ops). This allows the future drivers like virtio-mdev to implement its own
> device specific ops.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> .../driver-api/vfio-mediated-device.rst | 4 +-
> MAINTAINERS | 1 +
> drivers/gpu/drm/i915/gvt/kvmgt.c | 15 +++---
> drivers/s390/cio/vfio_ccw_ops.c | 15 ++++--
> drivers/s390/crypto/vfio_ap_ops.c | 11 ++--
> drivers/vfio/mdev/vfio_mdev.c | 31 ++++++-----
> include/linux/mdev.h | 36 ++-----------
> include/linux/vfio_mdev.h | 53 +++++++++++++++++++
> samples/vfio-mdev/mbochs.c | 17 +++---
> samples/vfio-mdev/mdpy.c | 17 +++---
> samples/vfio-mdev/mtty.c | 15 ++++--
> 11 files changed, 138 insertions(+), 77 deletions(-) create mode 100644
> include/linux/vfio_mdev.h
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> b/Documentation/driver-api/vfio-mediated-device.rst
> index 0e052072e1d8..3ab00e48212f 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -152,7 +152,9 @@ callbacks per mdev parent device, per mdev type, or
> any other categorization.
> Vendor drivers are expected to be fully asynchronous in this respect or
> provide their own internal resource protection.)
>
> -The callbacks in the mdev_parent_ops structure are as follows:
> +The device specific callbacks are referred through device_ops pointer
> +in mdev_parent_ops. For vfio-mdev device, its callbacks in device_ops
> +are as follows:
>
> * open: open callback of mediated device
> * close: close callback of mediated device diff --git a/MAINTAINERS
> b/MAINTAINERS index b2326dece28e..89832b316500 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17075,6 +17075,7 @@ S: Maintained
> F: Documentation/driver-api/vfio-mediated-device.rst
> F: drivers/vfio/mdev/
> F: include/linux/mdev.h
> +F: include/linux/vfio_mdev.h
> F: samples/vfio-mdev/
>
> VFIO PLATFORM DRIVER
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 19d51a35f019..8ea86b1e69f1 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -42,6 +42,7 @@
> #include <linux/kvm_host.h>
> #include <linux/vfio.h>
> #include <linux/mdev.h>
> +#include <linux/vfio_mdev.h>
> #include <linux/debugfs.h>
>
> #include <linux/nospec.h>
> @@ -1600,20 +1601,22 @@ static const struct attribute_group
> *intel_vgpu_groups[] = {
> NULL,
> };
>
> -static struct mdev_parent_ops intel_vgpu_ops = {
> - .mdev_attr_groups = intel_vgpu_groups,
> - .create = intel_vgpu_create,
> - .remove = intel_vgpu_remove,
> -
> +static struct vfio_mdev_parent_ops intel_vfio_vgpu_ops = {
Naming it with _dev prefix as intel_vfio_vgpu_dev_ops is better to differentiate with parent_ops.
> .open = intel_vgpu_open,
> .release = intel_vgpu_release,
> -
> .read = intel_vgpu_read,
> .write = intel_vgpu_write,
> .mmap = intel_vgpu_mmap,
> .ioctl = intel_vgpu_ioctl,
> };
>
> +static struct mdev_parent_ops intel_vgpu_ops = {
> + .mdev_attr_groups = intel_vgpu_groups,
> + .create = intel_vgpu_create,
> + .remove = intel_vgpu_remove,
> + .device_ops = &intel_vfio_vgpu_ops,
> +};
> +
> static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops) {
> struct attribute **kvm_type_attrs;
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 246ff0f80944..02122bbc213e 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -12,6 +12,7 @@
>
> #include <linux/vfio.h>
> #include <linux/mdev.h>
> +#include <linux/vfio_mdev.h>
> #include <linux/nospec.h>
> #include <linux/slab.h>
>
> @@ -574,11 +575,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct
> mdev_device *mdev,
> }
> }
>
> -static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> - .owner = THIS_MODULE,
> - .supported_type_groups = mdev_type_groups,
> - .create = vfio_ccw_mdev_create,
> - .remove = vfio_ccw_mdev_remove,
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> .open = vfio_ccw_mdev_open,
> .release = vfio_ccw_mdev_release,
> .read = vfio_ccw_mdev_read,
> @@ -586,6 +583,14 @@ static const struct mdev_parent_ops
> vfio_ccw_mdev_ops = {
> .ioctl = vfio_ccw_mdev_ioctl,
> };
>
> +static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> + .owner = THIS_MODULE,
> + .supported_type_groups = mdev_type_groups,
> + .create = vfio_ccw_mdev_create,
> + .remove = vfio_ccw_mdev_remove,
> + .device_ops = &vfio_mdev_ops,
> +};
> +
> int vfio_ccw_mdev_reg(struct subchannel *sch) {
> return mdev_register_vfio_device(&sch->dev, &vfio_ccw_mdev_ops);
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> b/drivers/s390/crypto/vfio_ap_ops.c
> index 7487fc39d2c5..4251becc7a6d 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -16,6 +16,7 @@
> #include <linux/bitops.h>
> #include <linux/kvm_host.h>
> #include <linux/module.h>
> +#include <linux/vfio_mdev.h>
> #include <asm/kvm.h>
> #include <asm/zcrypt.h>
>
> @@ -1280,15 +1281,19 @@ static ssize_t vfio_ap_mdev_ioctl(struct
> mdev_device *mdev,
> return ret;
> }
>
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> + .open = vfio_ap_mdev_open,
> + .release = vfio_ap_mdev_release,
> + .ioctl = vfio_ap_mdev_ioctl,
> +};
> +
> static const struct mdev_parent_ops vfio_ap_matrix_ops = {
> .owner = THIS_MODULE,
> .supported_type_groups = vfio_ap_mdev_type_groups,
> .mdev_attr_groups = vfio_ap_mdev_attr_groups,
> .create = vfio_ap_mdev_create,
> .remove = vfio_ap_mdev_remove,
> - .open = vfio_ap_mdev_open,
> - .release = vfio_ap_mdev_release,
> - .ioctl = vfio_ap_mdev_ioctl,
> + .device_ops = &vfio_mdev_ops,
> };
>
> int vfio_ap_mdev_register(void)
> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> index fd2a4d9a3f26..d23c9f58c84f 100644
> --- a/drivers/vfio/mdev/vfio_mdev.c
> +++ b/drivers/vfio/mdev/vfio_mdev.c
> @@ -14,6 +14,7 @@
> #include <linux/slab.h>
> #include <linux/vfio.h>
> #include <linux/mdev.h>
> +#include <linux/vfio_mdev.h>
>
> #include "mdev_private.h"
>
> @@ -25,15 +26,16 @@ static int vfio_mdev_open(void *device_data) {
> struct mdev_device *mdev = device_data;
> struct mdev_parent *parent = mdev->parent;
> + const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
> int ret;
>
> - if (unlikely(!parent->ops->open))
> + if (unlikely(!ops->open))
> return -EINVAL;
>
device_ops is optional and can be NULL for mdev devices which are not required to be mapped via vfio.
So please change to,
If (!ops || !ops->open)
return -EINVAL;
and rest of the below places.
> if (!try_module_get(THIS_MODULE))
> return -ENODEV;
>
> - ret = parent->ops->open(mdev);
> + ret = ops->open(mdev);
> if (ret)
> module_put(THIS_MODULE);
>
> @@ -44,9 +46,10 @@ static void vfio_mdev_release(void *device_data) {
> struct mdev_device *mdev = device_data;
> struct mdev_parent *parent = mdev->parent;
> + const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>
> - if (likely(parent->ops->release))
> - parent->ops->release(mdev);
> + if (likely(ops->release))
> + ops->release(mdev);
>
> module_put(THIS_MODULE);
> }
> @@ -56,11 +59,12 @@ static long vfio_mdev_unlocked_ioctl(void
> *device_data, {
> struct mdev_device *mdev = device_data;
> struct mdev_parent *parent = mdev->parent;
> + const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>
> - if (unlikely(!parent->ops->ioctl))
> + if (unlikely(!ops->ioctl))
> return -EINVAL;
>
> - return parent->ops->ioctl(mdev, cmd, arg);
> + return ops->ioctl(mdev, cmd, arg);
> }
>
> static ssize_t vfio_mdev_read(void *device_data, char __user *buf, @@ -68,11
> +72,12 @@ static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
> {
> struct mdev_device *mdev = device_data;
> struct mdev_parent *parent = mdev->parent;
> + const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>
> - if (unlikely(!parent->ops->read))
> + if (unlikely(!ops->read))
> return -EINVAL;
>
> - return parent->ops->read(mdev, buf, count, ppos);
> + return ops->read(mdev, buf, count, ppos);
> }
>
> static ssize_t vfio_mdev_write(void *device_data, const char __user *buf, @@
> -80,22 +85,24 @@ static ssize_t vfio_mdev_write(void *device_data, const char
> __user *buf, {
> struct mdev_device *mdev = device_data;
> struct mdev_parent *parent = mdev->parent;
> + const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>
> - if (unlikely(!parent->ops->write))
> + if (unlikely(!ops->write))
> return -EINVAL;
>
> - return parent->ops->write(mdev, buf, count, ppos);
> + return ops->write(mdev, buf, count, ppos);
> }
>
> static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma) {
> struct mdev_device *mdev = device_data;
> struct mdev_parent *parent = mdev->parent;
> + const struct vfio_mdev_parent_ops *ops = parent->ops->device_ops;
>
> - if (unlikely(!parent->ops->mmap))
> + if (unlikely(!ops->mmap))
> return -EINVAL;
>
> - return parent->ops->mmap(mdev, vma);
> + return ops->mmap(mdev, vma);
> }
>
> static const struct vfio_device_ops vfio_mdev_dev_ops = { diff --git
> a/include/linux/mdev.h b/include/linux/mdev.h index
> 3ebae310f599..fa167bcb81e1 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -48,30 +48,8 @@ struct device *mdev_get_iommu_device(struct device
> *dev);
> * @mdev: mdev_device device structure which is being
> * destroyed
> * Returns integer: success (0) or error (< 0)
> - * @open: Open mediated device.
> - * @mdev: mediated device.
> - * Returns integer: success (0) or error (< 0)
> - * @release: release mediated device
> - * @mdev: mediated device.
> - * @read: Read emulation callback
> - * @mdev: mediated device structure
> - * @buf: read buffer
> - * @count: number of bytes to read
> - * @ppos: address.
> - * Retuns number on bytes read on success or error.
> - * @write: Write emulation callback
> - * @mdev: mediated device structure
> - * @buf: write buffer
> - * @count: number of bytes to be written
> - * @ppos: address.
> - * Retuns number on bytes written on success or error.
> - * @ioctl: IOCTL callback
> - * @mdev: mediated device structure
> - * @cmd: ioctl command
> - * @arg: arguments to ioctl
> - * @mmap: mmap callback
> - * @mdev: mediated device structure
> - * @vma: vma structure
> + * @device_ops: Device specific emulation callback.
> + *
> * Parent device that support mediated device should be registered with mdev
> * module with mdev_parent_ops structure.
> **/
> @@ -83,15 +61,7 @@ struct mdev_parent_ops {
>
> int (*create)(struct kobject *kobj, struct mdev_device *mdev);
> int (*remove)(struct mdev_device *mdev);
> - int (*open)(struct mdev_device *mdev);
> - void (*release)(struct mdev_device *mdev);
> - ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> - size_t count, loff_t *ppos);
> - ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> - size_t count, loff_t *ppos);
> - long (*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> - unsigned long arg);
> - int (*mmap)(struct mdev_device *mdev, struct vm_area_struct
> *vma);
> + const void *device_ops;
> };
>
> /* interface for exporting mdev supported type attributes */ diff --git
> a/include/linux/vfio_mdev.h b/include/linux/vfio_mdev.h new file mode
> 100644 index 000000000000..0c1b34f98f5d
> --- /dev/null
> +++ b/include/linux/vfio_mdev.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * VFIO Mediated device definition
> + */
> +
> +#ifndef VFIO_MDEV_H
> +#define VFIO_MDEV_H
> +
> +#include <linux/types.h>
> +#include <linux/mdev.h>
> +
> +/**
> + * struct vfio_mdev_parent_ops - Structure to be registered for each
> + * parent device to register the device to vfio-mdev module.
> + *
> + * @open: Open mediated device.
> + * @mdev: mediated device.
> + * Returns integer: success (0) or error (< 0)
> + * @release: release mediated device
> + * @mdev: mediated device.
> + * @read: Read emulation callback
> + * @mdev: mediated device structure
> + * @buf: read buffer
> + * @count: number of bytes to read
> + * @ppos: address.
> + * Retuns number on bytes read on success or error.
> + * @write: Write emulation callback
> + * @mdev: mediated device structure
> + * @buf: write buffer
> + * @count: number of bytes to be written
> + * @ppos: address.
> + * Retuns number on bytes written on success or error.
> + * @ioctl: IOCTL callback
> + * @mdev: mediated device structure
> + * @cmd: ioctl command
> + * @arg: arguments to ioctl
> + * @mmap: mmap callback
> + * @mdev: mediated device structure
> + * @vma: vma structure
> + */
> +struct vfio_mdev_parent_ops {
> + int (*open)(struct mdev_device *mdev);
> + void (*release)(struct mdev_device *mdev);
> + ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> + size_t count, loff_t *ppos);
> + ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> + size_t count, loff_t *ppos);
> + long (*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> + unsigned long arg);
> + int (*mmap)(struct mdev_device *mdev, struct vm_area_struct
> *vma);
> +};
> +
> +#endif
> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index
> 71a4469be85d..107cc30d0f45 100644
> --- a/samples/vfio-mdev/mbochs.c
> +++ b/samples/vfio-mdev/mbochs.c
> @@ -30,6 +30,7 @@
> #include <linux/iommu.h>
> #include <linux/sysfs.h>
> #include <linux/mdev.h>
> +#include <linux/vfio_mdev.h>
> #include <linux/pci.h>
> #include <linux/dma-buf.h>
> #include <linux/highmem.h>
> @@ -1418,12 +1419,7 @@ static struct attribute_group *mdev_type_groups[]
> = {
> NULL,
> };
>
> -static const struct mdev_parent_ops mdev_fops = {
> - .owner = THIS_MODULE,
> - .mdev_attr_groups = mdev_dev_groups,
> - .supported_type_groups = mdev_type_groups,
> - .create = mbochs_create,
> - .remove = mbochs_remove,
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> .open = mbochs_open,
> .release = mbochs_close,
> .read = mbochs_read,
> @@ -1432,6 +1428,15 @@ static const struct mdev_parent_ops mdev_fops = {
> .mmap = mbochs_mmap,
> };
>
> +static const struct mdev_parent_ops mdev_fops = {
> + .owner = THIS_MODULE,
> + .mdev_attr_groups = mdev_dev_groups,
> + .supported_type_groups = mdev_type_groups,
> + .create = mbochs_create,
> + .remove = mbochs_remove,
> + .device_ops = &vfio_mdev_ops,
> +};
> +
> static const struct file_operations vd_fops = {
> .owner = THIS_MODULE,
> };
> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c index
> d3029dd27d91..2cd2018a53f9 100644
> --- a/samples/vfio-mdev/mdpy.c
> +++ b/samples/vfio-mdev/mdpy.c
> @@ -26,6 +26,7 @@
> #include <linux/iommu.h>
> #include <linux/sysfs.h>
> #include <linux/mdev.h>
> +#include <linux/vfio_mdev.h>
> #include <linux/pci.h>
> #include <drm/drm_fourcc.h>
> #include "mdpy-defs.h"
> @@ -725,12 +726,7 @@ static struct attribute_group *mdev_type_groups[] = {
> NULL,
> };
>
> -static const struct mdev_parent_ops mdev_fops = {
> - .owner = THIS_MODULE,
> - .mdev_attr_groups = mdev_dev_groups,
> - .supported_type_groups = mdev_type_groups,
> - .create = mdpy_create,
> - .remove = mdpy_remove,
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> .open = mdpy_open,
> .release = mdpy_close,
> .read = mdpy_read,
> @@ -739,6 +735,15 @@ static const struct mdev_parent_ops mdev_fops = {
> .mmap = mdpy_mmap,
> };
>
> +static const struct mdev_parent_ops mdev_fops = {
> + .owner = THIS_MODULE,
> + .mdev_attr_groups = mdev_dev_groups,
> + .supported_type_groups = mdev_type_groups,
> + .create = mdpy_create,
> + .remove = mdpy_remove,
> + .device_ops = &vfio_mdev_ops,
> +};
> +
> static const struct file_operations vd_fops = {
> .owner = THIS_MODULE,
> };
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index
> 744c88a6b22c..e427425b5daf 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -27,6 +27,7 @@
> #include <linux/ctype.h>
> #include <linux/file.h>
> #include <linux/mdev.h>
> +#include <linux/vfio_mdev.h>
> #include <linux/pci.h>
> #include <linux/serial.h>
> #include <uapi/linux/serial_reg.h>
> @@ -1410,6 +1411,14 @@ static struct attribute_group *mdev_type_groups[]
> = {
> NULL,
> };
>
> +static const struct vfio_mdev_parent_ops vfio_mdev_ops = {
> + .open = mtty_open,
> + .release = mtty_close,
> + .read = mtty_read,
> + .write = mtty_write,
> + .ioctl = mtty_ioctl,
> +};
> +
> static const struct mdev_parent_ops mdev_fops = {
> .owner = THIS_MODULE,
> .dev_attr_groups = mtty_dev_groups,
> @@ -1417,11 +1426,7 @@ static const struct mdev_parent_ops mdev_fops = {
> .supported_type_groups = mdev_type_groups,
> .create = mtty_create,
> .remove = mtty_remove,
> - .open = mtty_open,
> - .release = mtty_close,
> - .read = mtty_read,
> - .write = mtty_write,
> - .ioctl = mtty_ioctl,
> + .device_ops = &vfio_mdev_ops,
> };
>
> static void mtty_device_release(struct device *dev)
> --
> 2.19.1
^ permalink raw reply
* Re: [RFC PATCH bpf-next v2 1/2] bpf: adding map batch processing support
From: Brian Vazquez @ 2019-09-23 23:20 UTC (permalink / raw)
To: Yonghong Song
Cc: ast, Daniel Borkmann, netdev, bpf, kernel-team, Jakub Kicinski,
Brian Vazquez, Stanislav Fomichev
In-Reply-To: <20190906225434.3635421-2-yhs@fb.com>
Hi Yonghong, thanks for working on this!
I have some concerns about this implementation but overall I think
this might work for our use case too!
On Sun, Sep 8, 2019 at 1:11 AM Yonghong Song <yhs@fb.com> wrote:
>
> Brian Vazquez has proposed BPF_MAP_DUMP command to look up more than one
> map entries per syscall.
> https://lore.kernel.org/bpf/CABCgpaU3xxX6CMMxD+1knApivtc2jLBHysDXw-0E9bQEL0qC3A@mail.gmail.com/T/#t
>
> During discussion, we found more use cases can be supported in a similar
> map operation batching framework. For example, batched map lookup and delete,
> which can be really helpful for bcc.
> https://github.com/iovisor/bcc/blob/master/tools/tcptop.py#L233-L243
> https://github.com/iovisor/bcc/blob/master/tools/slabratetop.py#L129-L138
>
> Also, in bcc, we have API to delete all entries in a map.
> https://github.com/iovisor/bcc/blob/master/src/cc/api/BPFTable.h#L257-L264
>
> For map update, batched operations also useful as sometimes applications need
> to populate initial maps with more than one entry. For example, the below
> example is from kernel/samples/bpf/xdp_redirect_cpu_user.c:
> https://github.com/torvalds/linux/blob/master/samples/bpf/xdp_redirect_cpu_user.c#L543-L550
>
> This patch addresses all the above use cases. To make uapi stable, it also
> covers other potential use cases. For bpf syscall subcommands are introduced:
> BPF_MAP_LOOKUP_BATCH
> BPF_MAP_LOOKUP_AND_DELETE_BATCH
> BPF_MAP_UPDATE_BATCH
> BPF_MAP_DELETE_BATCH
>
> The UAPI attribute structure looks like:
>
> struct { /* struct used by BPF_MAP_*_BATCH commands */
> __u64 batch; /* input/output:
> * input: start batch,
> * 0 to start from beginning.
> * output: next start batch,
> * 0 to end batching.
> */
> __aligned_u64 keys;
> __aligned_u64 values;
> __u32 count; /* input/output:
> * input: # of elements keys/values.
> * output: # of filled elements.
> */
> __u32 map_fd;
> __u64 elem_flags;
> __u64 flags;
> } batch;
>
> An opaque value 'batch' is used for user/kernel space communication
> for where in the map to start the operation for lookup/lookup_and_delete/delete.
> input 'batch' = 0: to start the operation from the beginning of the map.
> output 'batch': if not 0, the next input for batch operation.
>
> For lookup/lookup_and_delete:
> operation: lookup/lookup_and_delete starting from a particular 'batch'.
> return:
> 'batch' 'count' return code meaning
> 0 0 0 Done. Nothing left
> 0 0 -ENOSPC no space to handle batch 0
> > 0 0 -ENOSPC no space to handle 'batch'
> > 0 > 0 0 stopped right before 'batch'
> Note that:
> (1). Even if return code is 0 and return 'count' > 0, the return 'count' may
> not be equal to input 'count'. This happens when there is no enough space
> to handle a batch.
> (2). If the return code is an error and not -EFAULT,
> 'batch' indicates the batch has issues and 'count' indicates the number
> of elements successfully processed.
>
> For delete:
> operation: deletion starting from a particular 'batch'.
> return: 0 means everything is deleted from 'batch'.
> error code means something deletion not happening.
>
> For update:
> operation: update 'count' number of elements in 'keys'/'values'.
> return: 0 means successful updates for all elements.
> error code, if not -EFAULT, 'count' is the number of successful updates.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> include/linux/bpf.h | 9 ++
> include/uapi/linux/bpf.h | 22 +++
> kernel/bpf/hashtab.c | 324 +++++++++++++++++++++++++++++++++++++++
> kernel/bpf/syscall.c | 68 ++++++++
> 4 files changed, 423 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 5b9d22338606..3c1302e8e2d4 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -37,6 +37,15 @@ struct bpf_map_ops {
> int (*map_get_next_key)(struct bpf_map *map, void *key, void *next_key);
> void (*map_release_uref)(struct bpf_map *map);
> void *(*map_lookup_elem_sys_only)(struct bpf_map *map, void *key);
> + int (*map_lookup_batch)(struct bpf_map *map, const union bpf_attr *attr,
> + union bpf_attr __user *uattr);
> + int (*map_lookup_and_delete_batch)(struct bpf_map *map,
> + const union bpf_attr *attr,
> + union bpf_attr __user *uattr);
> + int (*map_update_batch)(struct bpf_map *map, const union bpf_attr *attr,
> + union bpf_attr __user *uattr);
> + int (*map_delete_batch)(struct bpf_map *map, const union bpf_attr *attr,
> + union bpf_attr __user *uattr);
>
> /* funcs callable from userspace and from eBPF programs */
> void *(*map_lookup_elem)(struct bpf_map *map, void *key);
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 5d2fb183ee2d..9d4f76073dd9 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -107,6 +107,10 @@ enum bpf_cmd {
> BPF_MAP_LOOKUP_AND_DELETE_ELEM,
> BPF_MAP_FREEZE,
> BPF_BTF_GET_NEXT_ID,
> + BPF_MAP_LOOKUP_BATCH,
> + BPF_MAP_LOOKUP_AND_DELETE_BATCH,
> + BPF_MAP_UPDATE_BATCH,
> + BPF_MAP_DELETE_BATCH,
> };
>
> enum bpf_map_type {
> @@ -396,6 +400,24 @@ union bpf_attr {
> __u64 flags;
> };
>
> + struct { /* struct used by BPF_MAP_*_BATCH commands */
> + __u64 batch; /* input/output:
> + * input: start batch,
> + * 0 to start from beginning.
> + * output: next start batch,
> + * 0 to end batching.
> + */
> + __aligned_u64 keys;
> + __aligned_u64 values;
> + __u32 count; /* input/output:
> + * input: # of elements keys/values.
> + * output: # of filled elements.
> + */
> + __u32 map_fd;
> + __u64 elem_flags;
> + __u64 flags;
> + } batch;
> +
> struct { /* anonymous struct used by BPF_PROG_LOAD command */
> __u32 prog_type; /* one of enum bpf_prog_type */
> __u32 insn_cnt;
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 22066a62c8c9..ee7b90200f4d 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -1232,6 +1232,322 @@ static void htab_map_seq_show_elem(struct bpf_map *map, void *key,
> rcu_read_unlock();
> }
>
> +static int
> +__htab_map_lookup_and_delete_batch(struct bpf_map *map,
> + const union bpf_attr *attr,
> + union bpf_attr __user *uattr,
> + bool do_delete, bool is_lru_map)
> +{
> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> + u32 bucket_cnt, total, key_size, value_size, roundup_key_size;
> + void *keys = NULL, *values = NULL, *value, *dst_key, *dst_val;
> + u64 elem_map_flags, map_flags;
> + struct hlist_nulls_head *head;
> + void __user *ukeys, *uvalues;
> + struct hlist_nulls_node *n;
> + u32 batch, max_count;
> + unsigned long flags;
> + struct htab_elem *l;
> + struct bucket *b;
> + int ret = 0;
> +
> + max_count = attr->batch.count;
> + if (!max_count)
> + return 0;
> +
> + elem_map_flags = attr->batch.elem_flags;
> + if ((elem_map_flags & ~BPF_F_LOCK) ||
> + ((elem_map_flags & BPF_F_LOCK) && !map_value_has_spin_lock(map)))
> + return -EINVAL;
> +
> + map_flags = attr->batch.flags;
> + if (map_flags)
> + return -EINVAL;
> +
> + batch = (u32)attr->batch.batch;
> + if (batch >= htab->n_buckets)
> + return -EINVAL;
> +
> + /* We cannot do copy_from_user or copy_to_user inside
> + * the rcu_read_lock. Allocate enough space here.
> + */
> + key_size = htab->map.key_size;
> + roundup_key_size = round_up(htab->map.key_size, 8);
> + value_size = htab->map.value_size;
> + keys = kvmalloc(key_size * max_count, GFP_USER | __GFP_NOWARN);
> + values = kvmalloc(value_size * max_count, GFP_USER | __GFP_NOWARN);
> + if (!keys || !values) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + dst_key = keys;
> + dst_val = values;
> + total = 0;
> +
> + preempt_disable();
> + this_cpu_inc(bpf_prog_active);
> + rcu_read_lock();
> +
> +again:
> + b = &htab->buckets[batch];
> + head = &b->head;
> + raw_spin_lock_irqsave(&b->lock, flags);
> +
Would it be possible to avoid that lock when we're not deleting (just
batching lookup)? To be honest I don't how much impact would have to
grab that lock when concurrent additions are happening in a bpf
program.
> + bucket_cnt = 0;
> + hlist_nulls_for_each_entry_rcu(l, n, head, hash_node)
> + bucket_cnt++;
> +
> + if (bucket_cnt > (max_count - total)) {
> + if (total == 0)
> + ret = -ENOSPC;
> + goto after_loop;
> + }
> +
> + hlist_nulls_for_each_entry_rcu(l, n, head, hash_node) {
> + memcpy(dst_key, l->key, key_size);
> +
> + value = l->key + roundup_key_size;
> + if (elem_map_flags & BPF_F_LOCK)
> + copy_map_value_locked(map, dst_val, value, true);
> + else
> + copy_map_value(map, dst_val, value);
> + check_and_init_map_lock(map, dst_val);
> +
> + dst_key += key_size;
> + dst_val += value_size;
> + total++;
> + }
> +
> + if (do_delete) {
> + hlist_nulls_for_each_entry_rcu(l, n, head, hash_node) {
> + hlist_nulls_del_rcu(&l->hash_node);
> + if (is_lru_map)
> + bpf_lru_push_free(&htab->lru, &l->lru_node);
> + else
> + free_htab_elem(htab, l);
> + }
> + }
> +
> + batch++;
> + if (batch >= htab->n_buckets) {
> + batch = 0;
> + goto after_loop;
> + }
> +
> + raw_spin_unlock_irqrestore(&b->lock, flags);
> + goto again;
> +
> +after_loop:
> + raw_spin_unlock_irqrestore(&b->lock, flags);
> +
> + rcu_read_unlock();
> + this_cpu_dec(bpf_prog_active);
> + preempt_enable();
> +
> + /* copy data back to user */
> + ukeys = u64_to_user_ptr(attr->batch.keys);
> + uvalues = u64_to_user_ptr(attr->batch.values);
> + if (put_user(batch, &uattr->batch.batch) ||
> + copy_to_user(ukeys, keys, total * key_size) ||
> + copy_to_user(uvalues, values, total * value_size) ||
> + put_user(total, &uattr->batch.count))
> + ret = -EFAULT;
> +
> +out:
> + kvfree(keys);
> + kvfree(values);
> + return ret;
> +}
> +
> +static int
> +__htab_map_update_batch(struct bpf_map *map, const union bpf_attr *attr,
> + union bpf_attr __user *uattr, bool is_lru_map)
> +{
> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> + u32 count, max_count, key_size, roundup_key_size, value_size;
> + u64 elem_map_flags, map_flags;
> + void __user *ukey, *uvalue;
> + void *key, *value;
> + int ret = 0;
> +
> + max_count = attr->batch.count;
> + if (!max_count)
> + return 0;
> +
> + elem_map_flags = attr->batch.elem_flags;
> + if ((elem_map_flags & BPF_F_LOCK) && !map_value_has_spin_lock(map))
> + return -EINVAL;
> +
> + map_flags = attr->batch.flags;
> + if (map_flags)
> + return -EINVAL;
> +
> + key_size = htab->map.key_size;
> + roundup_key_size = round_up(htab->map.key_size, 8);
> + value_size = htab->map.value_size;
> + key = kmalloc(key_size, GFP_USER | __GFP_NOWARN);
> + value = kmalloc(value_size, GFP_USER | __GFP_NOWARN);
> + if (!key || !value) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ukey = u64_to_user_ptr(attr->batch.keys);
> + uvalue = u64_to_user_ptr(attr->batch.values);
> + for (count = 0; count < max_count; count++) {
> + if (copy_from_user(key, ukey + count * key_size, key_size) ||
> + copy_from_user(value, uvalue + count * value_size, value_size)) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + preempt_disable();
> + __this_cpu_inc(bpf_prog_active);
> + rcu_read_lock();
> + if (is_lru_map)
> + ret = htab_lru_map_update_elem(map, key, value, elem_map_flags);
> + else
> + ret = htab_map_update_elem(map, key, value, elem_map_flags);
> + rcu_read_unlock();
> + __this_cpu_dec(bpf_prog_active);
> + preempt_enable();
> +
> + if (ret) {
> + if (put_user(count, &uattr->batch.count))
> + ret = -EFAULT;
> + break;
> + }
> + }
> +
> +out:
> + kfree(key);
> + kfree(value);
> + return ret;
> +}
> +
> +static int
> +__htab_map_delete_batch(struct bpf_map *map,
> + const union bpf_attr *attr,
> + union bpf_attr __user *uattr,
> + bool is_lru_map)
> +{
> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> + u64 elem_map_flags, map_flags;
> + struct hlist_nulls_head *head;
> + struct hlist_nulls_node *n;
> + u32 batch, max_count;
> + unsigned long flags;
> + struct htab_elem *l;
> + struct bucket *b;
> +
> + elem_map_flags = attr->batch.elem_flags;
> + map_flags = attr->batch.flags;
> + if (elem_map_flags || map_flags)
> + return -EINVAL;
> +
> + max_count = attr->batch.count;
> + batch = (u32)attr->batch.batch;
> + if (max_count || batch >= htab->n_buckets)
> + return -EINVAL;
> +
> + preempt_disable();
> + __this_cpu_inc(bpf_prog_active);
> + rcu_read_lock();
> +
> +again:
> + b = &htab->buckets[batch];
> + head = &b->head;
> + raw_spin_lock_irqsave(&b->lock, flags);
> +
> + hlist_nulls_for_each_entry_rcu(l, n, head, hash_node) {
> + hlist_nulls_del_rcu(&l->hash_node);
> + if (is_lru_map)
> + bpf_lru_push_free(&htab->lru, &l->lru_node);
> + else
> + free_htab_elem(htab, l);
> + }
> +
> + batch++;
> + if (batch >= htab->n_buckets)
> + goto out;
> +
> + raw_spin_unlock_irqrestore(&b->lock, flags);
> + goto again;
> +
> +out:
> + raw_spin_unlock_irqrestore(&b->lock, flags);
> + rcu_read_unlock();
> + __this_cpu_dec(bpf_prog_active);
> + preempt_enable();
> +
> + return 0;
> +}
> +
> +static int
> +htab_map_lookup_batch(struct bpf_map *map, const union bpf_attr *attr,
> + union bpf_attr __user *uattr)
> +{
> + return __htab_map_lookup_and_delete_batch(map, attr, uattr, false,
> + false);
> +}
> +
> +static int
> +htab_map_lookup_and_delete_batch(struct bpf_map *map,
> + const union bpf_attr *attr,
> + union bpf_attr __user *uattr)
> +{
> + return __htab_map_lookup_and_delete_batch(map, attr, uattr, true,
> + false);
> +}
> +
> +static int
> +htab_map_update_batch(struct bpf_map *map, const union bpf_attr *attr,
> + union bpf_attr __user *uattr)
> +{
> + return __htab_map_update_batch(map, attr, uattr, false);
> +}
> +
> +static int
> +htab_map_delete_batch(struct bpf_map *map,
> + const union bpf_attr *attr,
> + union bpf_attr __user *uattr)
> +{
> + return __htab_map_delete_batch(map, attr, uattr, false);
> +}
> +
> +static int
> +htab_lru_map_lookup_batch(struct bpf_map *map, const union bpf_attr *attr,
> + union bpf_attr __user *uattr)
> +{
> + return __htab_map_lookup_and_delete_batch(map, attr, uattr, false,
> + true);
> +}
> +
> +static int
> +htab_lru_map_lookup_and_delete_batch(struct bpf_map *map,
> + const union bpf_attr *attr,
> + union bpf_attr __user *uattr)
> +{
> + return __htab_map_lookup_and_delete_batch(map, attr, uattr, true,
> + true);
> +}
> +
> +static int
> +htab_lru_map_update_batch(struct bpf_map *map, const union bpf_attr *attr,
> + union bpf_attr __user *uattr)
> +{
> + return __htab_map_update_batch(map, attr, uattr, true);
> +}
> +
> +static int
> +htab_lru_map_delete_batch(struct bpf_map *map,
> + const union bpf_attr *attr,
> + union bpf_attr __user *uattr)
> +{
> + return __htab_map_delete_batch(map, attr, uattr, true);
> +}
> +
> const struct bpf_map_ops htab_map_ops = {
> .map_alloc_check = htab_map_alloc_check,
> .map_alloc = htab_map_alloc,
> @@ -1242,6 +1558,10 @@ const struct bpf_map_ops htab_map_ops = {
> .map_delete_elem = htab_map_delete_elem,
> .map_gen_lookup = htab_map_gen_lookup,
> .map_seq_show_elem = htab_map_seq_show_elem,
> + .map_lookup_batch = htab_map_lookup_batch,
> + .map_lookup_and_delete_batch = htab_map_lookup_and_delete_batch,
> + .map_update_batch = htab_map_update_batch,
> + .map_delete_batch = htab_map_delete_batch,
> };
>
> const struct bpf_map_ops htab_lru_map_ops = {
> @@ -1255,6 +1575,10 @@ const struct bpf_map_ops htab_lru_map_ops = {
> .map_delete_elem = htab_lru_map_delete_elem,
> .map_gen_lookup = htab_lru_map_gen_lookup,
> .map_seq_show_elem = htab_map_seq_show_elem,
> + .map_lookup_batch = htab_lru_map_lookup_batch,
> + .map_lookup_and_delete_batch = htab_lru_map_lookup_and_delete_batch,
> + .map_update_batch = htab_lru_map_update_batch,
> + .map_delete_batch = htab_lru_map_delete_batch,
> };
>
> /* Called from eBPF program */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index ca60eafa6922..e83bdf7efbd8 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2816,6 +2816,62 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
> return err;
> }
>
> +#define BPF_MAP_BATCH_LAST_FIELD batch.flags
> +
> +#define BPF_DO_BATCH(fn) \
> + do { \
> + if (!fn) { \
> + err = -ENOTSUPP; \
> + goto err_put; \
> + } \
> + err = fn(map, attr, uattr); \
> + } while(0)
> +
> +static int bpf_map_do_batch(const union bpf_attr *attr,
> + union bpf_attr __user *uattr,
> + int cmd)
> +{
> + struct bpf_map *map;
> + int err, ufd;
> + struct fd f;
> +
> + if (CHECK_ATTR(BPF_MAP_BATCH))
> + return -EINVAL;
> +
> + ufd = attr->batch.map_fd;
> + f = fdget(ufd);
> + map = __bpf_map_get(f);
> + if (IS_ERR(map))
> + return PTR_ERR(map);
> +
> + if ((cmd == BPF_MAP_LOOKUP_BATCH ||
> + cmd == BPF_MAP_LOOKUP_AND_DELETE_BATCH) &&
> + !(map_get_sys_perms(map, f) & FMODE_CAN_READ)) {
> + err = -EPERM;
> + goto err_put;
> + }
> +
> + if (cmd != BPF_MAP_LOOKUP_BATCH &&
> + !(map_get_sys_perms(map, f) & FMODE_CAN_WRITE)) {
> + err = -EPERM;
> + goto err_put;
> + }
> +
> + if (cmd == BPF_MAP_LOOKUP_BATCH) {
> + BPF_DO_BATCH(map->ops->map_lookup_batch);
> + } else if (cmd == BPF_MAP_LOOKUP_AND_DELETE_BATCH) {
> + BPF_DO_BATCH(map->ops->map_lookup_and_delete_batch);
> + } else if (cmd == BPF_MAP_UPDATE_BATCH) {
> + BPF_DO_BATCH(map->ops->map_update_batch);
> + } else {
> + BPF_DO_BATCH(map->ops->map_delete_batch);
> + }
> +
> +err_put:
> + fdput(f);
> + return err;
> +}
> +
> SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
> {
> union bpf_attr attr = {};
> @@ -2913,6 +2969,18 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
> case BPF_MAP_LOOKUP_AND_DELETE_ELEM:
> err = map_lookup_and_delete_elem(&attr);
> break;
> + case BPF_MAP_LOOKUP_BATCH:
> + err = bpf_map_do_batch(&attr, uattr, BPF_MAP_LOOKUP_BATCH);
> + break;
> + case BPF_MAP_LOOKUP_AND_DELETE_BATCH:
> + err = bpf_map_do_batch(&attr, uattr, BPF_MAP_LOOKUP_AND_DELETE_BATCH);
> + break;
> + case BPF_MAP_UPDATE_BATCH:
> + err = bpf_map_do_batch(&attr, uattr, BPF_MAP_UPDATE_BATCH);
> + break;
> + case BPF_MAP_DELETE_BATCH:
> + err = bpf_map_do_batch(&attr, uattr, BPF_MAP_DELETE_BATCH);
> + break;
> default:
> err = -EINVAL;
> break;
> --
> 2.17.1
>
In general it'd be great if we could express the old functions
(get_next_key, lookup, delete) in terms of this new ones so we avoid
having twice the code.
Also to be honest I don't see how batching updates would be useful,
maybe just try to do batching lookup and lookup_and_delete?
^ permalink raw reply
* [PATCH net v2] net/sched: cbs: Fix not adding cbs instance to list
From: Vinicius Costa Gomes @ 2019-09-24 0:15 UTC (permalink / raw)
To: netdev; +Cc: Vinicius Costa Gomes, jhs, xiyou.wangcong, jiri, davem
When removing a cbs instance when offloading is enabled, the crash
below can be observed.
The problem happens because that when offloading is enabled, the cbs
instance is not added to the list.
Also, the current code doesn't handle correctly the case when offload
is disabled without removing the qdisc: if the link speed changes the
credit calculations will be wrong. When we create the cbs instance
with offloading enabled, it's not added to the notification list, when
later we disable offloading, it's not in the list, so link speed
changes will not affect it.
The solution for both issues is the same, add the cbs instance being
created unconditionally to the global list, even if the link state
notification isn't useful "right now".
Crash log:
[518758.189866] BUG: kernel NULL pointer dereference, address: 0000000000000000
[518758.189870] #PF: supervisor read access in kernel mode
[518758.189871] #PF: error_code(0x0000) - not-present page
[518758.189872] PGD 0 P4D 0
[518758.189874] Oops: 0000 [#1] SMP PTI
[518758.189876] CPU: 3 PID: 4825 Comm: tc Not tainted 5.2.9 #1
[518758.189877] Hardware name: Gigabyte Technology Co., Ltd. Z390 AORUS ULTRA/Z390 AORUS ULTRA-CF, BIOS F7 03/14/2019
[518758.189881] RIP: 0010:__list_del_entry_valid+0x29/0xa0
[518758.189883] Code: 90 48 b8 00 01 00 00 00 00 ad de 55 48 8b 17 4c 8b 47 08 48 89 e5 48 39 c2 74 27 48 b8 00 02 00 00 00 00 ad de 49 39 c0 74 2d <49> 8b 30 48 39 fe 75 3d 48 8b 52 08 48 39 f2 75 4c b8 01 00 00 00
[518758.189885] RSP: 0018:ffffa27e43903990 EFLAGS: 00010207
[518758.189887] RAX: dead000000000200 RBX: ffff8bce69f0f000 RCX: 0000000000000000
[518758.189888] RDX: 0000000000000000 RSI: ffff8bce69f0f064 RDI: ffff8bce69f0f1e0
[518758.189890] RBP: ffffa27e43903990 R08: 0000000000000000 R09: ffff8bce69e788c0
[518758.189891] R10: ffff8bce62acd400 R11: 00000000000003cb R12: ffff8bce69e78000
[518758.189892] R13: ffff8bce69f0f140 R14: 0000000000000000 R15: 0000000000000000
[518758.189894] FS: 00007fa1572c8f80(0000) GS:ffff8bce6e0c0000(0000) knlGS:0000000000000000
[518758.189895] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[518758.189896] CR2: 0000000000000000 CR3: 000000040a398006 CR4: 00000000003606e0
[518758.189898] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[518758.189899] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[518758.189900] Call Trace:
[518758.189904] cbs_destroy+0x32/0xa0 [sch_cbs]
[518758.189906] qdisc_destroy+0x45/0x120
[518758.189907] qdisc_put+0x25/0x30
[518758.189908] qdisc_graft+0x2c1/0x450
[518758.189910] tc_get_qdisc+0x1c8/0x310
[518758.189912] ? get_page_from_freelist+0x91a/0xcb0
[518758.189914] rtnetlink_rcv_msg+0x293/0x360
[518758.189916] ? kmem_cache_alloc_node_trace+0x178/0x260
[518758.189918] ? __kmalloc_node_track_caller+0x38/0x50
[518758.189920] ? rtnl_calcit.isra.0+0xf0/0xf0
[518758.189922] netlink_rcv_skb+0x48/0x110
[518758.189923] rtnetlink_rcv+0x10/0x20
[518758.189925] netlink_unicast+0x15b/0x1d0
[518758.189926] netlink_sendmsg+0x1ea/0x380
[518758.189929] sock_sendmsg+0x2f/0x40
[518758.189930] ___sys_sendmsg+0x295/0x2f0
[518758.189932] ? ___sys_recvmsg+0x151/0x1e0
[518758.189933] ? do_wp_page+0x7e/0x450
[518758.189935] __sys_sendmsg+0x48/0x80
[518758.189937] __x64_sys_sendmsg+0x1a/0x20
[518758.189939] do_syscall_64+0x53/0x1f0
[518758.189941] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[518758.189942] RIP: 0033:0x7fa15755169a
[518758.189944] Code: 48 c7 c0 ff ff ff ff eb be 0f 1f 80 00 00 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 18 b8 2e 00 00 00 c5 fc 77 0f 05 <48> 3d 00 f0 ff ff 77 5e c3 0f 1f 44 00 00 48 83 ec 28 89 54 24 1c
[518758.189946] RSP: 002b:00007ffda58b60b8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[518758.189948] RAX: ffffffffffffffda RBX: 000055e4b836d9a0 RCX: 00007fa15755169a
[518758.189949] RDX: 0000000000000000 RSI: 00007ffda58b6128 RDI: 0000000000000003
[518758.189951] RBP: 00007ffda58b6190 R08: 0000000000000001 R09: 000055e4b9d848a0
[518758.189952] R10: 0000000000000000 R11: 0000000000000246 R12: 000000005d654b49
[518758.189953] R13: 0000000000000000 R14: 00007ffda58b6230 R15: 00007ffda58b6210
[518758.189955] Modules linked in: sch_cbs sch_etf sch_mqprio netlink_diag unix_diag e1000e igb intel_pch_thermal thermal video backlight pcc_cpufreq
[518758.189960] CR2: 0000000000000000
[518758.189961] ---[ end trace 6a13f7aaf5376019 ]---
[518758.189963] RIP: 0010:__list_del_entry_valid+0x29/0xa0
[518758.189964] Code: 90 48 b8 00 01 00 00 00 00 ad de 55 48 8b 17 4c 8b 47 08 48 89 e5 48 39 c2 74 27 48 b8 00 02 00 00 00 00 ad de 49 39 c0 74 2d <49> 8b 30 48 39 fe 75 3d 48 8b 52 08 48 39 f2 75 4c b8 01 00 00 00
[518758.189967] RSP: 0018:ffffa27e43903990 EFLAGS: 00010207
[518758.189968] RAX: dead000000000200 RBX: ffff8bce69f0f000 RCX: 0000000000000000
[518758.189969] RDX: 0000000000000000 RSI: ffff8bce69f0f064 RDI: ffff8bce69f0f1e0
[518758.189971] RBP: ffffa27e43903990 R08: 0000000000000000 R09: ffff8bce69e788c0
[518758.189972] R10: ffff8bce62acd400 R11: 00000000000003cb R12: ffff8bce69e78000
[518758.189973] R13: ffff8bce69f0f140 R14: 0000000000000000 R15: 0000000000000000
[518758.189975] FS: 00007fa1572c8f80(0000) GS:ffff8bce6e0c0000(0000) knlGS:0000000000000000
[518758.189976] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[518758.189977] CR2: 0000000000000000 CR3: 000000040a398006 CR4: 00000000003606e0
[518758.189979] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[518758.189980] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Fixes: e0a7683 ("net/sched: cbs: fix port_rate miscalculation")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
Changes from v1:
- Expanded commit message.
net/sched/sch_cbs.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
index 93b58fd..fc15c1b 100644
--- a/net/sched/sch_cbs.c
+++ b/net/sched/sch_cbs.c
@@ -404,6 +404,10 @@ static int cbs_init(struct Qdisc *sch, struct nlattr *opt,
if (!q->qdisc)
return -ENOMEM;
+ spin_lock(&cbs_list_lock);
+ list_add(&q->cbs_list, &cbs_list);
+ spin_unlock(&cbs_list_lock);
+
qdisc_hash_add(q->qdisc, false);
q->queue = sch->dev_queue - netdev_get_tx_queue(dev, 0);
@@ -417,12 +421,6 @@ static int cbs_init(struct Qdisc *sch, struct nlattr *opt,
if (err)
return err;
- if (!q->offload) {
- spin_lock(&cbs_list_lock);
- list_add(&q->cbs_list, &cbs_list);
- spin_unlock(&cbs_list_lock);
- }
-
return 0;
}
@@ -431,15 +429,18 @@ static void cbs_destroy(struct Qdisc *sch)
struct cbs_sched_data *q = qdisc_priv(sch);
struct net_device *dev = qdisc_dev(sch);
- spin_lock(&cbs_list_lock);
- list_del(&q->cbs_list);
- spin_unlock(&cbs_list_lock);
+ /* Nothing to do if we couldn't create the underlying qdisc */
+ if (!q->qdisc)
+ return;
qdisc_watchdog_cancel(&q->watchdog);
cbs_disable_offload(dev, q);
- if (q->qdisc)
- qdisc_put(q->qdisc);
+ spin_lock(&cbs_list_lock);
+ list_del(&q->cbs_list);
+ spin_unlock(&cbs_list_lock);
+
+ qdisc_put(q->qdisc);
}
static int cbs_dump(struct Qdisc *sch, struct sk_buff *skb)
--
2.23.0
^ permalink raw reply related
* Re: [PATCH 1/1] MAINTAINERS: add Yanjun to FORCEDETH maintainers list
From: Zhu Yanjun @ 2019-09-24 0:23 UTC (permalink / raw)
To: rain.1986.08.12, mchehab+samsung, davem, gregkh, robh,
linus.walleij, nicolas.ferre, linux-kernel, netdev
In-Reply-To: <20190923143746.4310-1-rain.1986.08.12@gmail.com>
On 2019/9/23 22:37, rain.1986.08.12@gmail.com wrote:
> From: Rain River <rain.1986.08.12@gmail.com>
>
> Yanjun has been spending quite a lot of time fixing bugs
> in FORCEDETH source code. I'd like to add Yanjun to maintainers
> list.
>
> Signed-off-by: Rain River <rain.1986.08.12@gmail.com>
Acked-by: Zhu Yanjun <yanjun.zhu@oracle.com>
Thanks a lot.
Zhu Yanjun
> ---
> MAINTAINERS | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a400af0501c9..336ad8fe8b60 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -643,6 +643,7 @@ F: drivers/net/ethernet/alacritech/*
>
> FORCEDETH GIGABIT ETHERNET DRIVER
> M: Rain River <rain.1986.08.12@gmail.com>
> +M: Zhu Yanjun <yanjun.zhu@oracle.com>
> L: netdev@vger.kernel.org
> S: Maintained
> F: drivers/net/ethernet/nvidia/*
^ permalink raw reply
* Re: [PATCH V2 net 1/1] net/tls(TLS_SW): Fix list_del double free caused by a race condition in tls_tx_records
From: Jakub Kicinski @ 2019-09-24 0:28 UTC (permalink / raw)
To: Pooja Trivedi
Cc: netdev, davem, daniel, john.fastabend, davejwatson, aviadye,
borisp, Pooja Trivedi, Mallesham Jatharakonda
In-Reply-To: <CAOrEdsk6P=HWfK-mKyLt7=tZh342gZrRKwOH9f6ntkNyya-4fA@mail.gmail.com>
On Sat, 21 Sep 2019 23:19:20 -0400, Pooja Trivedi wrote:
> On Wed, Sep 18, 2019 at 5:45 PM Jakub Kicinski wrote:
> > On Wed, 18 Sep 2019 17:37:44 -0400, Pooja Trivedi wrote:
> > > Hi Jakub,
> > >
> > > I have explained one potential way for the race to happen in my
> > > original message to the netdev mailing list here:
> > > https://marc.info/?l=linux-netdev&m=156805120229554&w=2
> > >
> > > Here is the part out of there that's relevant to your question:
> > >
> > > -----------------------------------------
> > >
> > > One potential way for race condition to appear:
> > >
> > > When under tcp memory pressure, Thread 1 takes the following code path:
> > > do_sendfile ---> ... ---> .... ---> tls_sw_sendpage --->
> > > tls_sw_do_sendpage ---> tls_tx_records ---> tls_push_sg --->
> > > do_tcp_sendpages ---> sk_stream_wait_memory ---> sk_wait_event
> >
> > Ugh, so do_tcp_sendpages() can also release the lock :/
> >
> > Since the problem occurs in tls_sw_do_sendpage() and
> > tls_sw_do_sendmsg() as well, should we perhaps fix it at that level?
>
> That won't do because tls_tx_records also gets called when completion
> callbacks schedule delayed work. That was the code path that caused
> the crash for my test. Cavium's nitrox crypto offload driver calling
> tls_encrypt_done, which calls schedule_delayed_work. Delayed work that
> was scheduled would then be processed by tx_work_handler.
> Notice in my previous reply,
> "Thread 2 code path:
> tx_work_handler ---> tls_tx_records"
>
> "Thread 2 code path:
> tx_work_handler ---> tls_tx_records"
Right, the work handler would obviously also have to obey the exclusion
mechanism of choice.
Having said that this really does feel like we are trying to lock code,
not data here :(
^ permalink raw reply
* RE: [PATCH] rtlwifi: rtl8723ae: Remove unused 'rtstatus' variable
From: Pkshih @ 2019-09-24 1:21 UTC (permalink / raw)
To: Austin Kim, kvalo@codeaurora.org, davem@davemloft.net
Cc: linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CADLLry5b1RDjXX8Dbc4ebbZOFFaAd0wc3rDCaD-V9RBwrpNyMA@mail.gmail.com>
> -----Original Message-----
> From: Austin Kim [mailto:austindh.kim@gmail.com]
> Sent: Monday, September 23, 2019 9:35 PM
> To: Pkshih; kvalo@codeaurora.org; davem@davemloft.net
> Cc: linux-wireless@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] rtlwifi: rtl8723ae: Remove unused 'rtstatus' variable
>
> Hello, Maintainers...
> Would you please review above patch if you are available?
>
You can check status of your patch here
https://patchwork.kernel.org/project/linux-wireless/list/?series=174859
If your patch is existing in patchwork, maintainer doesn't forget it.
Another thing is to avoid top posting that makes maintainer hard to read mail.
---
PK
^ permalink raw reply
* RE: [PATCH] r8152: Use guard clause and fix comment typos
From: Hayes Wang @ 2019-09-24 2:47 UTC (permalink / raw)
To: Prashant Malani; +Cc: grundler@chromium.org, netdev@vger.kernel.org, nic_swsd
In-Reply-To: <20190923222657.253628-1-pmalani@chromium.org>
Prashant Malani [mailto:pmalani@chromium.org]
> Sent: Tuesday, September 24, 2019 6:27 AM
> To: Hayes Wang
[...]
> - do {
> + while (1) {
> struct tx_agg *agg;
> + struct net_device *netdev = tp->netdev;
>
> if (skb_queue_empty(&tp->tx_queue))
> break;
> @@ -2188,26 +2189,25 @@ static void tx_bottom(struct r8152 *tp)
> break;
>
> res = r8152_tx_agg_fill(tp, agg);
> - if (res) {
> - struct net_device *netdev = tp->netdev;
> + if (!res)
> + break;
I let the loop run continually until an error occurs or the queue is empty.
However, you stop the loop when r8152_tx_agg_fill() is successful.
If an error occurs continually, the loop may not be broken.
> - if (res == -ENODEV) {
> - rtl_set_unplug(tp);
> - netif_device_detach(netdev);
> - } else {
> - struct net_device_stats *stats = &netdev->stats;
> - unsigned long flags;
> + if (res == -ENODEV) {
> + rtl_set_unplug(tp);
> + netif_device_detach(netdev);
> + } else {
> + struct net_device_stats *stats = &netdev->stats;
> + unsigned long flags;
>
> - netif_warn(tp, tx_err, netdev,
> - "failed tx_urb %d\n", res);
> - stats->tx_dropped += agg->skb_num;
> + netif_warn(tp, tx_err, netdev,
> + "failed tx_urb %d\n", res);
> + stats->tx_dropped += agg->skb_num;
>
> - spin_lock_irqsave(&tp->tx_lock, flags);
> - list_add_tail(&agg->list, &tp->tx_free);
> - spin_unlock_irqrestore(&tp->tx_lock, flags);
> - }
> + spin_lock_irqsave(&tp->tx_lock, flags);
> + list_add_tail(&agg->list, &tp->tx_free);
> + spin_unlock_irqrestore(&tp->tx_lock, flags);
> }
> - } while (res == 0);
> + }
I think the behavior is different from the current one.
Best Regards,
Hayes
^ permalink raw reply
* [PATCH v2] r8152: Use guard clause and fix comment typos
From: Prashant Malani @ 2019-09-24 4:00 UTC (permalink / raw)
To: hayeswang; +Cc: grundler, netdev, nic_swsd, Prashant Malani
Use a guard clause in tx_bottom() to reduce the indentation of the
do-while loop.
Also, fix a couple of spelling and grammatical mistakes in the
r8152_csum_workaround() function comment.
Change-Id: I460befde150ad92248fd85b0f189ec2df2ab8431
Signed-off-by: Prashant Malani <pmalani@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
---
drivers/net/usb/r8152.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 08726090570e1..2c1faa8cf5fc9 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1688,7 +1688,7 @@ static struct tx_agg *r8152_get_tx_agg(struct r8152 *tp)
}
/* r8152_csum_workaround()
- * The hw limites the value the transport offset. When the offset is out of the
+ * The hw limits the value of the transport offset. When the offset is out of
* range, calculate the checksum by sw.
*/
static void r8152_csum_workaround(struct r8152 *tp, struct sk_buff *skb,
@@ -2179,6 +2179,7 @@ static void tx_bottom(struct r8152 *tp)
do {
struct tx_agg *agg;
+ struct net_device *netdev = tp->netdev;
if (skb_queue_empty(&tp->tx_queue))
break;
@@ -2188,24 +2189,23 @@ static void tx_bottom(struct r8152 *tp)
break;
res = r8152_tx_agg_fill(tp, agg);
- if (res) {
- struct net_device *netdev = tp->netdev;
+ if (!res)
+ continue;
- if (res == -ENODEV) {
- rtl_set_unplug(tp);
- netif_device_detach(netdev);
- } else {
- struct net_device_stats *stats = &netdev->stats;
- unsigned long flags;
+ if (res == -ENODEV) {
+ rtl_set_unplug(tp);
+ netif_device_detach(netdev);
+ } else {
+ struct net_device_stats *stats = &netdev->stats;
+ unsigned long flags;
- netif_warn(tp, tx_err, netdev,
- "failed tx_urb %d\n", res);
- stats->tx_dropped += agg->skb_num;
+ netif_warn(tp, tx_err, netdev,
+ "failed tx_urb %d\n", res);
+ stats->tx_dropped += agg->skb_num;
- spin_lock_irqsave(&tp->tx_lock, flags);
- list_add_tail(&agg->list, &tp->tx_free);
- spin_unlock_irqrestore(&tp->tx_lock, flags);
- }
+ spin_lock_irqsave(&tp->tx_lock, flags);
+ list_add_tail(&agg->list, &tp->tx_free);
+ spin_unlock_irqrestore(&tp->tx_lock, flags);
}
} while (res == 0);
}
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related
* Re: [PATCH net v2] net/sched: cbs: Fix not adding cbs instance to list
From: Cong Wang @ 2019-09-24 4:22 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <20190924001502.22384-1-vinicius.gomes@intel.com>
On Mon, Sep 23, 2019 at 5:14 PM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
> @@ -417,12 +421,6 @@ static int cbs_init(struct Qdisc *sch, struct nlattr *opt,
> if (err)
> return err;
>
> - if (!q->offload) {
> - spin_lock(&cbs_list_lock);
> - list_add(&q->cbs_list, &cbs_list);
> - spin_unlock(&cbs_list_lock);
> - }
> -
> return 0;
These two return's now can be folded into one, right?
^ permalink raw reply
* Re: [PATCH net v2] net/sched: cbs: Fix not adding cbs instance to list
From: Vinicius Costa Gomes @ 2019-09-24 4:43 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <CAM_iQpVsNdqQPY5p8ZaN6_soBx+TuTT_vjfYWod7LrSuw3ufeA@mail.gmail.com>
Hi,
Cong Wang <xiyou.wangcong@gmail.com> writes:
> On Mon, Sep 23, 2019 at 5:14 PM Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>> @@ -417,12 +421,6 @@ static int cbs_init(struct Qdisc *sch, struct nlattr *opt,
>> if (err)
>> return err;
>>
>> - if (!q->offload) {
>> - spin_lock(&cbs_list_lock);
>> - list_add(&q->cbs_list, &cbs_list);
>> - spin_unlock(&cbs_list_lock);
>> - }
>> -
>> return 0;
>
> These two return's now can be folded into one, right?
Yeah, good catch. Will fix.
Cheers,
--
Vinicius
^ permalink raw reply
* [PATCH net v3] net/sched: cbs: Fix not adding cbs instance to list
From: Vinicius Costa Gomes @ 2019-09-24 5:04 UTC (permalink / raw)
To: netdev; +Cc: Vinicius Costa Gomes, jhs, xiyou.wangcong, jiri, davem
When removing a cbs instance when offloading is enabled, the crash
below can be observed.
The problem happens because that when offloading is enabled, the cbs
instance is not added to the list.
Also, the current code doesn't handle correctly the case when offload
is disabled without removing the qdisc: if the link speed changes the
credit calculations will be wrong. When we create the cbs instance
with offloading enabled, it's not added to the notification list, when
later we disable offloading, it's not in the list, so link speed
changes will not affect it.
The solution for both issues is the same, add the cbs instance being
created unconditionally to the global list, even if the link state
notification isn't useful "right now".
Crash log:
[518758.189866] BUG: kernel NULL pointer dereference, address: 0000000000000000
[518758.189870] #PF: supervisor read access in kernel mode
[518758.189871] #PF: error_code(0x0000) - not-present page
[518758.189872] PGD 0 P4D 0
[518758.189874] Oops: 0000 [#1] SMP PTI
[518758.189876] CPU: 3 PID: 4825 Comm: tc Not tainted 5.2.9 #1
[518758.189877] Hardware name: Gigabyte Technology Co., Ltd. Z390 AORUS ULTRA/Z390 AORUS ULTRA-CF, BIOS F7 03/14/2019
[518758.189881] RIP: 0010:__list_del_entry_valid+0x29/0xa0
[518758.189883] Code: 90 48 b8 00 01 00 00 00 00 ad de 55 48 8b 17 4c 8b 47 08 48 89 e5 48 39 c2 74 27 48 b8 00 02 00 00 00 00 ad de 49 39 c0 74 2d <49> 8b 30 48 39 fe 75 3d 48 8b 52 08 48 39 f2 75 4c b8 01 00 00 00
[518758.189885] RSP: 0018:ffffa27e43903990 EFLAGS: 00010207
[518758.189887] RAX: dead000000000200 RBX: ffff8bce69f0f000 RCX: 0000000000000000
[518758.189888] RDX: 0000000000000000 RSI: ffff8bce69f0f064 RDI: ffff8bce69f0f1e0
[518758.189890] RBP: ffffa27e43903990 R08: 0000000000000000 R09: ffff8bce69e788c0
[518758.189891] R10: ffff8bce62acd400 R11: 00000000000003cb R12: ffff8bce69e78000
[518758.189892] R13: ffff8bce69f0f140 R14: 0000000000000000 R15: 0000000000000000
[518758.189894] FS: 00007fa1572c8f80(0000) GS:ffff8bce6e0c0000(0000) knlGS:0000000000000000
[518758.189895] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[518758.189896] CR2: 0000000000000000 CR3: 000000040a398006 CR4: 00000000003606e0
[518758.189898] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[518758.189899] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[518758.189900] Call Trace:
[518758.189904] cbs_destroy+0x32/0xa0 [sch_cbs]
[518758.189906] qdisc_destroy+0x45/0x120
[518758.189907] qdisc_put+0x25/0x30
[518758.189908] qdisc_graft+0x2c1/0x450
[518758.189910] tc_get_qdisc+0x1c8/0x310
[518758.189912] ? get_page_from_freelist+0x91a/0xcb0
[518758.189914] rtnetlink_rcv_msg+0x293/0x360
[518758.189916] ? kmem_cache_alloc_node_trace+0x178/0x260
[518758.189918] ? __kmalloc_node_track_caller+0x38/0x50
[518758.189920] ? rtnl_calcit.isra.0+0xf0/0xf0
[518758.189922] netlink_rcv_skb+0x48/0x110
[518758.189923] rtnetlink_rcv+0x10/0x20
[518758.189925] netlink_unicast+0x15b/0x1d0
[518758.189926] netlink_sendmsg+0x1ea/0x380
[518758.189929] sock_sendmsg+0x2f/0x40
[518758.189930] ___sys_sendmsg+0x295/0x2f0
[518758.189932] ? ___sys_recvmsg+0x151/0x1e0
[518758.189933] ? do_wp_page+0x7e/0x450
[518758.189935] __sys_sendmsg+0x48/0x80
[518758.189937] __x64_sys_sendmsg+0x1a/0x20
[518758.189939] do_syscall_64+0x53/0x1f0
[518758.189941] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[518758.189942] RIP: 0033:0x7fa15755169a
[518758.189944] Code: 48 c7 c0 ff ff ff ff eb be 0f 1f 80 00 00 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 18 b8 2e 00 00 00 c5 fc 77 0f 05 <48> 3d 00 f0 ff ff 77 5e c3 0f 1f 44 00 00 48 83 ec 28 89 54 24 1c
[518758.189946] RSP: 002b:00007ffda58b60b8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[518758.189948] RAX: ffffffffffffffda RBX: 000055e4b836d9a0 RCX: 00007fa15755169a
[518758.189949] RDX: 0000000000000000 RSI: 00007ffda58b6128 RDI: 0000000000000003
[518758.189951] RBP: 00007ffda58b6190 R08: 0000000000000001 R09: 000055e4b9d848a0
[518758.189952] R10: 0000000000000000 R11: 0000000000000246 R12: 000000005d654b49
[518758.189953] R13: 0000000000000000 R14: 00007ffda58b6230 R15: 00007ffda58b6210
[518758.189955] Modules linked in: sch_cbs sch_etf sch_mqprio netlink_diag unix_diag e1000e igb intel_pch_thermal thermal video backlight pcc_cpufreq
[518758.189960] CR2: 0000000000000000
[518758.189961] ---[ end trace 6a13f7aaf5376019 ]---
[518758.189963] RIP: 0010:__list_del_entry_valid+0x29/0xa0
[518758.189964] Code: 90 48 b8 00 01 00 00 00 00 ad de 55 48 8b 17 4c 8b 47 08 48 89 e5 48 39 c2 74 27 48 b8 00 02 00 00 00 00 ad de 49 39 c0 74 2d <49> 8b 30 48 39 fe 75 3d 48 8b 52 08 48 39 f2 75 4c b8 01 00 00 00
[518758.189967] RSP: 0018:ffffa27e43903990 EFLAGS: 00010207
[518758.189968] RAX: dead000000000200 RBX: ffff8bce69f0f000 RCX: 0000000000000000
[518758.189969] RDX: 0000000000000000 RSI: ffff8bce69f0f064 RDI: ffff8bce69f0f1e0
[518758.189971] RBP: ffffa27e43903990 R08: 0000000000000000 R09: ffff8bce69e788c0
[518758.189972] R10: ffff8bce62acd400 R11: 00000000000003cb R12: ffff8bce69e78000
[518758.189973] R13: ffff8bce69f0f140 R14: 0000000000000000 R15: 0000000000000000
[518758.189975] FS: 00007fa1572c8f80(0000) GS:ffff8bce6e0c0000(0000) knlGS:0000000000000000
[518758.189976] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[518758.189977] CR2: 0000000000000000 CR3: 000000040a398006 CR4: 00000000003606e0
[518758.189979] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[518758.189980] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Fixes: e0a7683 ("net/sched: cbs: fix port_rate miscalculation")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
Changes from v2:
- Simplification to cbs_init() (Cong Wang)
Changes from v1:
- Expanded commit message.
net/sched/sch_cbs.c | 30 +++++++++++++-----------------
1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
index 93b58fd..1bef152 100644
--- a/net/sched/sch_cbs.c
+++ b/net/sched/sch_cbs.c
@@ -392,7 +392,6 @@ static int cbs_init(struct Qdisc *sch, struct nlattr *opt,
{
struct cbs_sched_data *q = qdisc_priv(sch);
struct net_device *dev = qdisc_dev(sch);
- int err;
if (!opt) {
NL_SET_ERR_MSG(extack, "Missing CBS qdisc options which are mandatory");
@@ -404,6 +403,10 @@ static int cbs_init(struct Qdisc *sch, struct nlattr *opt,
if (!q->qdisc)
return -ENOMEM;
+ spin_lock(&cbs_list_lock);
+ list_add(&q->cbs_list, &cbs_list);
+ spin_unlock(&cbs_list_lock);
+
qdisc_hash_add(q->qdisc, false);
q->queue = sch->dev_queue - netdev_get_tx_queue(dev, 0);
@@ -413,17 +416,7 @@ static int cbs_init(struct Qdisc *sch, struct nlattr *opt,
qdisc_watchdog_init(&q->watchdog, sch);
- err = cbs_change(sch, opt, extack);
- if (err)
- return err;
-
- if (!q->offload) {
- spin_lock(&cbs_list_lock);
- list_add(&q->cbs_list, &cbs_list);
- spin_unlock(&cbs_list_lock);
- }
-
- return 0;
+ return cbs_change(sch, opt, extack);
}
static void cbs_destroy(struct Qdisc *sch)
@@ -431,15 +424,18 @@ static void cbs_destroy(struct Qdisc *sch)
struct cbs_sched_data *q = qdisc_priv(sch);
struct net_device *dev = qdisc_dev(sch);
- spin_lock(&cbs_list_lock);
- list_del(&q->cbs_list);
- spin_unlock(&cbs_list_lock);
+ /* Nothing to do if we couldn't create the underlying qdisc */
+ if (!q->qdisc)
+ return;
qdisc_watchdog_cancel(&q->watchdog);
cbs_disable_offload(dev, q);
- if (q->qdisc)
- qdisc_put(q->qdisc);
+ spin_lock(&cbs_list_lock);
+ list_del(&q->cbs_list);
+ spin_unlock(&cbs_list_lock);
+
+ qdisc_put(q->qdisc);
}
static int cbs_dump(struct Qdisc *sch, struct sk_buff *skb)
--
2.23.0
^ permalink raw reply related
* Re: general protection fault in xsk_map_update_elem
From: Björn Töpel @ 2019-09-24 5:05 UTC (permalink / raw)
To: Daniel Borkmann, syzbot
Cc: ast, bpf, davem, hawk, jakub.kicinski, john.fastabend,
jonathan.lemon, kafai, linux-kernel, magnus.karlsson, netdev,
songliubraving, syzkaller-bugs, yhs
In-Reply-To: <20190923225050.GA26406@pc-63.home>
On 2019-09-24 00:50, Daniel Borkmann wrote:
> On Mon, Sep 23, 2019 at 08:49:11AM -0700, syzbot wrote:
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit: b41dae06 Merge tag 'xfs-5.4-merge-7' of git://git.kernel.o..
>> git tree: net-next
>> console output: https://syzkaller.appspot.com/x/log.txt?x=130b25ad600000
>> kernel config: https://syzkaller.appspot.com/x/.config?x=dfcf592db22b9132
>> dashboard link: https://syzkaller.appspot.com/bug?extid=491c1b7565ba9069ecae
>> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
>> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=155a0c29600000
>> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=172bf6d9600000
>>
>> The bug was bisected to:
>>
>> commit 0402acd683c678874df6bdbc23530ca07ea19353
>> Author: Björn Töpel <bjorn.topel@intel.com>
>> Date: Thu Aug 15 09:30:13 2019 +0000
>>
>> xsk: remove AF_XDP socket from map when the socket is released
>
> Bjorn, PTAL, thanks.
>
I'm on it! Thanks for the poke.
^ permalink raw reply
* Re: [PATCH net v3] net/sched: cbs: Fix not adding cbs instance to list
From: Cong Wang @ 2019-09-24 5:13 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <20190924050458.14223-1-vinicius.gomes@intel.com>
On Mon, Sep 23, 2019 at 10:04 PM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> When removing a cbs instance when offloading is enabled, the crash
> below can be observed.
>
> The problem happens because that when offloading is enabled, the cbs
> instance is not added to the list.
>
> Also, the current code doesn't handle correctly the case when offload
> is disabled without removing the qdisc: if the link speed changes the
> credit calculations will be wrong. When we create the cbs instance
> with offloading enabled, it's not added to the notification list, when
> later we disable offloading, it's not in the list, so link speed
> changes will not affect it.
>
> The solution for both issues is the same, add the cbs instance being
> created unconditionally to the global list, even if the link state
> notification isn't useful "right now".
>
...
> Fixes: e0a7683 ("net/sched: cbs: fix port_rate miscalculation")
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
^ permalink raw reply
* Re: [PATCH trivial 2/2] drivers: net: Fix Kconfig indentation
From: Kalle Valo @ 2019-09-24 5:15 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David S. Miller, Michael Grzeschik, Wolfgang Grandegger,
Marc Kleine-Budde, Andrew Lunn, Florian Fainelli, Heiner Kallweit,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Pablo Neira Ayuso,
Jozsef Kadlecsik, Florian Westphal, Jiri Kosina, netdev,
linux-kernel, linux-can, linux-rdma, linux-wireless, b.a.t.m.a.n,
netfilter-devel, coreteam, lvs-devel, rds-devel
In-Reply-To: <20190923155243.6997-2-krzk@kernel.org>
Krzysztof Kozlowski <krzk@kernel.org> writes:
> Adjust indentation from spaces to tab (+optional two spaces) as in
> coding style with command like:
> $ sed -e 's/^ /\t/' -i */Kconfig
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
[...]
> drivers/net/wireless/ath/Kconfig | 2 +-
> drivers/net/wireless/ath/ar5523/Kconfig | 4 +-
> drivers/net/wireless/ath/ath6kl/Kconfig | 2 +-
> drivers/net/wireless/ath/ath9k/Kconfig | 2 +-
> drivers/net/wireless/ath/carl9170/Kconfig | 6 +-
> drivers/net/wireless/atmel/Kconfig | 32 ++---
> drivers/net/wireless/intel/ipw2x00/Kconfig | 116 +++++++++---------
> drivers/net/wireless/intel/iwlegacy/Kconfig | 6 +-
> drivers/net/wireless/intel/iwlwifi/Kconfig | 6 +-
> drivers/net/wireless/ralink/rt2x00/Kconfig | 24 ++--
I hope this goes through net or net-next, less chances of conflits then.
Acked-by: Kalle Valo <kvalo@codeaurora.org>
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH trivial 2/2] drivers: net: Fix Kconfig indentation
From: Leon Romanovsky @ 2019-09-24 7:03 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David S. Miller, Michael Grzeschik, Wolfgang Grandegger,
Marc Kleine-Budde, Andrew Lunn, Florian Fainelli, Heiner Kallweit,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Pablo Neira Ayuso,
Jozsef Kadlecsik, Florian Westphal, Jiri Kosina, netdev,
linux-kernel, linux-can, linux-rdma, linux-wireless, b.a.t.m.a.n,
netfilter-devel, coreteam, lvs-devel, rds-devel
In-Reply-To: <20190923155243.6997-2-krzk@kernel.org>
On Mon, Sep 23, 2019 at 05:52:43PM +0200, Krzysztof Kozlowski wrote:
> Adjust indentation from spaces to tab (+optional two spaces) as in
> coding style with command like:
> $ sed -e 's/^ /\t/' -i */Kconfig
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
<--->
> .../net/ethernet/mellanox/mlx5/core/Kconfig | 36 +++---
Thanks,
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
^ permalink raw reply
* [PATCH] ipv6: do not free rt if FIB_LOOKUP_NOREF is set on suppress rule
From: Jason A. Donenfeld @ 2019-09-24 7:36 UTC (permalink / raw)
To: netdev, linux-kernel; +Cc: Jason A. Donenfeld, stable
Commit 7d9e5f422150 removed references from certain dsts, but accounting
for this never translated down into the fib6 suppression code. This bug
was triggered by WireGuard users who use wg-quick(8), which uses the
"suppress-prefix" directive to ip-rule(8) for routing all of their
internet traffic without routing loops. The test case in the link of
this commit reliably triggers various crashes due to the use-after-free
caused by the reference underflow.
Cc: stable@vger.kernel.org
Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
Test-case: https://git.zx2c4.com/WireGuard/commit/?id=ad66532000f7a20b149e47c5eb3a957362c8e161
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
net/ipv6/fib6_rules.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index d22b6c140f23..f9e8fe3ff0c5 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -287,7 +287,8 @@ static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg
return false;
suppress_route:
- ip6_rt_put(rt);
+ if (!(arg->flags & FIB_LOOKUP_NOREF))
+ ip6_rt_put(rt);
return true;
}
--
2.21.0
^ permalink raw reply related
* ethtool 5.2 qsfp.c heap buffer overflow
From: Qiwei Wen @ 2019-09-24 7:41 UTC (permalink / raw)
To: netdev
Hi,
The function "sff8636_dom_parse", called from "sff8636_show_dom",
disregards the module eeprom size returned from the driver and assumes
the existence of upper pages, e.g.
sd->sfp_temp[HALRM] = SFF8636_OFFSET_TO_TEMP(SFF8636_TEMP_HALRM);
To reproduce: return ETH_MODULE_SFF_8636_LEN (256) for module eeprom
length in NIC driver, compile ethtool 5.2 with clang and address
sanitizer linked in, run ethtool -m.
Best regards,
Dave
^ permalink raw reply
* WARNING in mark_lock (2)
From: syzbot @ 2019-09-24 8:06 UTC (permalink / raw)
To: linux-fsdevel, linux-kernel, netdev, syzkaller-bugs, viro
Hello,
syzbot found the following crash on:
HEAD commit: b41dae06 Merge tag 'xfs-5.4-merge-7' of git://git.kernel.o..
git tree: net-next
console output: https://syzkaller.appspot.com/x/log.txt?x=16b6d8e9600000
kernel config: https://syzkaller.appspot.com/x/.config?x=dfcf592db22b9132
dashboard link: https://syzkaller.appspot.com/bug?extid=ef8a7c8214359d969c69
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+ef8a7c8214359d969c69@syzkaller.appspotmail.com
------------[ cut here ]------------
DEBUG_LOCKS_WARN_ON(1)
WARNING: CPU: 1 PID: 20740 at kernel/locking/lockdep.c:167 hlock_class
kernel/locking/lockdep.c:167 [inline]
WARNING: CPU: 1 PID: 20740 at kernel/locking/lockdep.c:167 hlock_class
kernel/locking/lockdep.c:156 [inline]
WARNING: CPU: 1 PID: 20740 at kernel/locking/lockdep.c:167
mark_lock+0x22b/0x1220 kernel/locking/lockdep.c:3643
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 20740 Comm: kworker/u4:10 Not tainted 5.3.0+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: writeback wb_workfn (flush-8:0)
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
panic+0x2dc/0x755 kernel/panic.c:219
__warn.cold+0x20/0x4c kernel/panic.c:576
report_bug+0x263/0x2b0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:179 [inline]
fixup_bug arch/x86/kernel/traps.c:174 [inline]
do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:272
do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:291
invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028
RIP: 0010:hlock_class kernel/locking/lockdep.c:167 [inline]
RIP: 0010:hlock_class kernel/locking/lockdep.c:156 [inline]
RIP: 0010:mark_lock+0x22b/0x1220 kernel/locking/lockdep.c:3643
Code: d0 7c 08 84 d2 0f 85 a8 0e 00 00 44 8b 1d 9d 6d 6e 08 45 85 db 75 b6
48 c7 c6 a0 26 ac 87 48 c7 c7 e0 26 ac 87 e8 2d 79 eb ff <0f> 0b 31 db e9
aa fe ff ff 48 c7 c7 e0 60 ae 8a e8 30 8e 54 00 e9
RSP: 0018:ffff88805d72f8d8 EFLAGS: 00010086
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff815c26d6 RDI: ffffed100bae5f0d
RBP: ffff88805d72f928 R08: ffff88806aefa480 R09: fffffbfff11f40b9
R10: fffffbfff11f40b8 R11: ffffffff88fa05c3 R12: 0000000000000008
R13: ffff88806aefad68 R14: 0000000000000000 R15: 00000000000c095a
mark_usage kernel/locking/lockdep.c:3592 [inline]
__lock_acquire+0x538/0x4e70 kernel/locking/lockdep.c:3909
lock_acquire+0x190/0x410 kernel/locking/lockdep.c:4487
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x95/0xcd kernel/locking/spinlock.c:159
__wake_up_common_lock+0xc8/0x150 kernel/sched/wait.c:122
__wake_up+0xe/0x10 kernel/sched/wait.c:142
finish_writeback_work.isra.0+0xf6/0x120 fs/fs-writeback.c:168
wb_do_writeback fs/fs-writeback.c:2030 [inline]
wb_workfn+0x34f/0x11e0 fs/fs-writeback.c:2070
process_one_work+0x9af/0x1740 kernel/workqueue.c:2269
worker_thread+0x98/0xe40 kernel/workqueue.c:2415
kthread+0x361/0x430 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply
* Re: [PATCH bpf-next 1/5] perf/core: Add PERF_FORMAT_LOST read_format
From: Jiri Olsa @ 2019-09-24 8:33 UTC (permalink / raw)
To: Daniel Xu
Cc: bpf, songliubraving, yhs, andriin, peterz, mingo, acme, ast,
alexander.shishkin, namhyung, linux-kernel, netdev, kernel-team
In-Reply-To: <20190917133056.5545-2-dxu@dxuuu.xyz>
On Tue, Sep 17, 2019 at 06:30:52AM -0700, Daniel Xu wrote:
SNIP
> + PERF_FORMAT_MAX = 1U << 5, /* non-ABI */
> };
>
> #define PERF_ATTR_SIZE_VER0 64 /* sizeof first published struct */
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 0463c1151bae..ee08d3ed6299 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1715,6 +1715,9 @@ static void __perf_event_read_size(struct perf_event *event, int nr_siblings)
> if (event->attr.read_format & PERF_FORMAT_ID)
> entry += sizeof(u64);
>
> + if (event->attr.read_format & PERF_FORMAT_LOST)
> + entry += sizeof(u64);
> +
> if (event->attr.read_format & PERF_FORMAT_GROUP) {
> nr += nr_siblings;
> size += sizeof(u64);
> @@ -4734,6 +4737,24 @@ u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
> }
> EXPORT_SYMBOL_GPL(perf_event_read_value);
>
> +static struct pmu perf_kprobe;
> +static u64 perf_event_lost(struct perf_event *event)
> +{
> + struct ring_buffer *rb;
> + u64 lost = 0;
> +
> + rcu_read_lock();
> + rb = rcu_dereference(event->rb);
> + if (likely(!!rb))
> + lost += local_read(&rb->lost);
> + rcu_read_unlock();
> +
> + if (event->attr.type == perf_kprobe.type)
> + lost += perf_kprobe_missed(event);
not sure what was the peterz's suggestion, but here you are mixing
ring buffer's lost count with kprobes missed count, seems wrong
maybe we could add PERF_FORMAT_KPROBE_MISSED
jirka
^ permalink raw reply
* [PATCH] net: print proper warning on dst underflow
From: Jason A. Donenfeld @ 2019-09-24 9:09 UTC (permalink / raw)
To: netdev, linux-kernel; +Cc: Jason A. Donenfeld
Proper warnings with stack traces make it much easier to figure out
what's doing the double free and create more meaningful bug reports from
users.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
net/core/dst.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/core/dst.c b/net/core/dst.c
index 1325316d9eab..193af526e908 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -172,7 +172,7 @@ void dst_release(struct dst_entry *dst)
int newrefcnt;
newrefcnt = atomic_dec_return(&dst->__refcnt);
- if (unlikely(newrefcnt < 0))
+ if (WARN_ONCE(newrefcnt < 0, "dst_release underflow"))
net_warn_ratelimited("%s: dst:%p refcnt:%d\n",
__func__, dst, newrefcnt);
if (!newrefcnt)
@@ -187,7 +187,7 @@ void dst_release_immediate(struct dst_entry *dst)
int newrefcnt;
newrefcnt = atomic_dec_return(&dst->__refcnt);
- if (unlikely(newrefcnt < 0))
+ if (WARN_ONCE(newrefcnt < 0, "dst_release_immediate underflow"))
net_warn_ratelimited("%s: dst:%p refcnt:%d\n",
__func__, dst, newrefcnt);
if (!newrefcnt)
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
From: Dan Carpenter @ 2019-09-24 9:21 UTC (permalink / raw)
To: Parav Pandit
Cc: netdev@vger.kernel.org, Leon Romanovsky, Eli Cohen, Doug Ledford,
Jason Gunthorpe, linux-rdma@vger.kernel.org,
kernel-janitors@vger.kernel.org
In-Reply-To: <AM4PR0501MB2260DF0BBBC528A147F07E0DD13D0@AM4PR0501MB2260.eurprd05.prod.outlook.com>
On Thu, Apr 25, 2019 at 06:15:13AM +0000, Parav Pandit wrote:
>
>
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Sent: Wednesday, April 24, 2019 9:08 AM
> > To: Parav Pandit <parav@mellanox.com>; netdev@vger.kernel.org
> > Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> > Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
> >
> > I think I'm just going to ask netdev for an opinion on this. It could be that
> > we're just reading the code wrong...
> >
> > I'm getting a lot of Smatch warning about buffer underflows. The problem is
> > that Smatch marks everything from nla_data() as unknown and untrusted
> > user data. In do_setvfinfo() we get the "->vf" values from nla_data(). It
> > starts as u32, but all the function pointers in net_device_ops use it as a
> > signed integer. Most of the functions return -EINVAL if "vf" is negative but
> > there are at least 48 which potentially use negative values as an offset into
> > an array.
> >
> > To me making "vf" a u32 throughout seems like a good idea but it's an
> > extensive patch and I'm not really able to test it at all.
>
> I will be try to get you patch early next week for core and in mlx5,
> tested on mlx5 VFs, that possibly you can carry forward?
Whatever happened with this?
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH stable 4.4 net] net: rds: Fix NULL ptr use in rds_tcp_kill_sock
From: maowenan @ 2019-09-24 9:33 UTC (permalink / raw)
To: Greg KH
Cc: chien.yen, davem, stable, rds-devel, netdev, linux-kernel,
kernel-janitors
In-Reply-To: <c8953355-2b98-c4d0-2af2-4a69ad3e2d2d@huawei.com>
Kindly ping...
On 2019/9/18 17:02, maowenan wrote:
>
>
> On 2019/9/18 16:32, Greg KH wrote:
>> On Wed, Sep 18, 2019 at 04:37:33PM +0800, Mao Wenan wrote:
>>> After the commit c4e97b06cfdc ("net: rds: force to destroy
>>> connection if t_sock is NULL in rds_tcp_kill_sock()."),
>>> it introduced null-ptr-deref in rds_tcp_kill_sock as below:
>>>
>>> BUG: KASAN: null-ptr-deref on address 0000000000000020
>>> Read of size 8 by task kworker/u16:10/910
>>> CPU: 3 PID: 910 Comm: kworker/u16:10 Not tainted 4.4.178+ #3
>>> Hardware name: linux,dummy-virt (DT)
>>> Workqueue: netns cleanup_net
>>> Call trace:
>>> [<ffffff90080abb50>] dump_backtrace+0x0/0x618
>>> [<ffffff90080ac1a0>] show_stack+0x38/0x60
>>> [<ffffff9008c42b78>] dump_stack+0x1a8/0x230
>>> [<ffffff90085d469c>] kasan_report_error+0xc8c/0xfc0
>>> [<ffffff90085d54a4>] kasan_report+0x94/0xd8
>>> [<ffffff90085d1b28>] __asan_load8+0x88/0x150
>>> [<ffffff9009c9cc2c>] rds_tcp_dev_event+0x734/0xb48
>>> [<ffffff90081eacb0>] raw_notifier_call_chain+0x150/0x1e8
>>> [<ffffff900973fec0>] call_netdevice_notifiers_info+0x90/0x110
>>> [<ffffff9009764874>] netdev_run_todo+0x2f4/0xb08
>>> [<ffffff9009796d34>] rtnl_unlock+0x2c/0x48
>>> [<ffffff9009756484>] default_device_exit_batch+0x444/0x528
>>> [<ffffff9009720498>] ops_exit_list+0x1c0/0x240
>>> [<ffffff9009724a80>] cleanup_net+0x738/0xbf8
>>> [<ffffff90081ca6cc>] process_one_work+0x96c/0x13e0
>>> [<ffffff90081cf370>] worker_thread+0x7e0/0x1910
>>> [<ffffff90081e7174>] kthread+0x304/0x390
>>> [<ffffff9008094280>] ret_from_fork+0x10/0x50
>>>
>>> If the first loop add the tc->t_sock = NULL to the tmp_list,
>>> 1). list_for_each_entry_safe(tc, _tc, &rds_tcp_conn_list, t_tcp_node)
>>>
>>> then the second loop is to find connections to destroy, tc->t_sock
>>> might equal NULL, and tc->t_sock->sk happens null-ptr-deref.
>>> 2). list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node)
>>>
>>> Fixes: c4e97b06cfdc ("net: rds: force to destroy connection if t_sock is NULL in rds_tcp_kill_sock().")
>>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>>> ---
>>> net/rds/tcp.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> Why is this not needed upstream as well?
> Upstream does not use tc->t_sock in the second loop after below two patches.
> afb4164d91c7 ("RDS: TCP: Refactor connection destruction to handle multiple paths") and
> 2d746c93b6e5 ("rds: tcp: remove redundant function rds_tcp_conn_paths_destroy()")
>
>>
>> 4.9.y? 4.14.y? anything else?
> 4.19.y and 4.14.y exist rds_tcp_conn_paths_destroy()
> to guarantee that.
> +static void rds_tcp_conn_paths_destroy(struct rds_connection *conn)
> +{
> + struct rds_conn_path *cp;
> + struct rds_tcp_connection *tc;
> + int i;
> + struct sock *sk;
> +
> + for (i = 0; i < RDS_MPATH_WORKERS; i++) {
> + cp = &conn->c_path[i];
> + tc = cp->cp_transport_data;
> + if (!tc->t_sock)
> + continue;
> + sk = tc->t_sock->sk;
> + sk->sk_prot->disconnect(sk, 0);
> + tcp_done(sk);
> + }
> +}
> +
>
>>
>> thanks,
>>
>> greg k-h
>>
>> .
>>
>
>
> .
>
^ permalink raw reply
* Re: [PATCH] ipv6: do not free rt if FIB_LOOKUP_NOREF is set on suppress rule
From: Jason A. Donenfeld @ 2019-09-24 9:35 UTC (permalink / raw)
To: Netdev, LKML, Wei Wang
In-Reply-To: <20190924073615.31704-1-Jason@zx2c4.com>
Hey Wei,
I meant to CC you but slipped up. Sorry about that. Take a look at
this thread if you have a chance.
Thanks,
Jason
On Tue, Sep 24, 2019 at 10:03 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Commit 7d9e5f422150 removed references from certain dsts, but accounting
> for this never translated down into the fib6 suppression code. This bug
> was triggered by WireGuard users who use wg-quick(8), which uses the
> "suppress-prefix" directive to ip-rule(8) for routing all of their
> internet traffic without routing loops. The test case in the link of
> this commit reliably triggers various crashes due to the use-after-free
> caused by the reference underflow.
>
> Cc: stable@vger.kernel.org
> Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
> Test-case: https://git.zx2c4.com/WireGuard/commit/?id=ad66532000f7a20b149e47c5eb3a957362c8e161
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> net/ipv6/fib6_rules.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
> index d22b6c140f23..f9e8fe3ff0c5 100644
> --- a/net/ipv6/fib6_rules.c
> +++ b/net/ipv6/fib6_rules.c
> @@ -287,7 +287,8 @@ static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg
> return false;
>
> suppress_route:
> - ip6_rt_put(rt);
> + if (!(arg->flags & FIB_LOOKUP_NOREF))
> + ip6_rt_put(rt);
> return true;
> }
>
> --
> 2.21.0
On Tue, Sep 24, 2019 at 10:03 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Commit 7d9e5f422150 removed references from certain dsts, but accounting
> for this never translated down into the fib6 suppression code. This bug
> was triggered by WireGuard users who use wg-quick(8), which uses the
> "suppress-prefix" directive to ip-rule(8) for routing all of their
> internet traffic without routing loops. The test case in the link of
> this commit reliably triggers various crashes due to the use-after-free
> caused by the reference underflow.
>
> Cc: stable@vger.kernel.org
> Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
> Test-case: https://git.zx2c4.com/WireGuard/commit/?id=ad66532000f7a20b149e47c5eb3a957362c8e161
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> net/ipv6/fib6_rules.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
> index d22b6c140f23..f9e8fe3ff0c5 100644
> --- a/net/ipv6/fib6_rules.c
> +++ b/net/ipv6/fib6_rules.c
> @@ -287,7 +287,8 @@ static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg
> return false;
>
> suppress_route:
> - ip6_rt_put(rt);
> + if (!(arg->flags & FIB_LOOKUP_NOREF))
> + ip6_rt_put(rt);
> return true;
> }
>
> --
> 2.21.0
--
Jason A. Donenfeld
Deep Space Explorer
fr: +33 6 51 90 82 66
us: +1 513 476 1200
^ permalink raw reply
* [pull request][net 0/7] Mellanox, mlx5 fixes 2019-09-24
From: Saeed Mahameed @ 2019-09-24 9:41 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev@vger.kernel.org, Saeed Mahameed
Hi Dave,
This series introduces some fixes to mlx5 driver.
For more information please see tag log below.
Please pull and let me know if there is any problem.
For -stable v4.10:
('net/mlx5e: Fix traffic duplication in ethtool steering')
For -stable v4.19:
('net/mlx5: Add device ID of upcoming BlueField-2')
For -stable v5.3:
('net/mlx5e: Fix matching on tunnel addresses type')
Thanks,
Saeed.
---
The following changes since commit 34b4688425d9841a19a15fa6ae2bfc12a372650f:
net: dsa: Use the correct style for SPDX License Identifier (2019-09-22 15:25:08 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git tags/mlx5-fixes-2019-09-24
for you to fetch changes up to fe1587a7de94912ed75ba5ddbfabf0741f9f8239:
net/mlx5e: Fix matching on tunnel addresses type (2019-09-24 12:38:08 +0300)
----------------------------------------------------------------
mlx5-fixes-2019-09-24
----------------------------------------------------------------
Alaa Hleihel (1):
net/mlx5: DR, Allow matching on vport based on vhca_id
Alex Vesker (2):
net/mlx5: DR, Remove redundant vport number from action
net/mlx5: DR, Fix getting incorrect prev node in ste_free
Bodong Wang (1):
net/mlx5: Add device ID of upcoming BlueField-2
Dmytro Linkin (1):
net/mlx5e: Fix matching on tunnel addresses type
Saeed Mahameed (1):
net/mlx5e: Fix traffic duplication in ethtool steering
Yevgeny Kliteynik (1):
net/mlx5: DR, Fix SW steering HW bits and definitions
.../ethernet/mellanox/mlx5/core/en_fs_ethtool.c | 4 +-
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 89 +++++++++++++---------
drivers/net/ethernet/mellanox/mlx5/core/main.c | 1 +
.../mellanox/mlx5/core/steering/dr_action.c | 4 +-
.../mellanox/mlx5/core/steering/dr_matcher.c | 13 ++--
.../ethernet/mellanox/mlx5/core/steering/dr_rule.c | 2 +-
.../ethernet/mellanox/mlx5/core/steering/dr_ste.c | 50 ++++++++----
.../mellanox/mlx5/core/steering/dr_types.h | 7 +-
include/linux/mlx5/mlx5_ifc.h | 28 +++----
9 files changed, 119 insertions(+), 79 deletions(-)
^ permalink raw reply
* [net 2/7] net/mlx5: DR, Remove redundant vport number from action
From: Saeed Mahameed @ 2019-09-24 9:41 UTC (permalink / raw)
To: David S. Miller
Cc: netdev@vger.kernel.org, Alex Vesker, Maor Gottlieb,
Saeed Mahameed
In-Reply-To: <20190924094047.15915-1-saeedm@mellanox.com>
From: Alex Vesker <valex@mellanox.com>
The vport number is part of the vport_cap, there is no reason
to store in a separate variable on the vport.
Fixes: 9db810ed2d37 ("net/mlx5: DR, Expose steering action functionality")
Signed-off-by: Alex Vesker <valex@mellanox.com>
Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
drivers/net/ethernet/mellanox/mlx5/core/steering/dr_action.c | 4 ++--
drivers/net/ethernet/mellanox/mlx5/core/steering/dr_types.h | 1 -
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_action.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_action.c
index 7d81a7735de5..b74b7d0f6590 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_action.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_action.c
@@ -615,7 +615,7 @@ static int dr_action_handle_cs_recalc(struct mlx5dr_domain *dmn,
* that recalculates the CS and forwards to the vport.
*/
ret = mlx5dr_domain_cache_get_recalc_cs_ft_addr(dest_action->vport.dmn,
- dest_action->vport.num,
+ dest_action->vport.caps->num,
final_icm_addr);
if (ret) {
mlx5dr_err(dmn, "Failed to get FW cs recalc flow table\n");
@@ -744,7 +744,7 @@ int mlx5dr_actions_build_ste_arr(struct mlx5dr_matcher *matcher,
dest_action = action;
if (rx_rule) {
/* Loopback on WIRE vport is not supported */
- if (action->vport.num == WIRE_PORT)
+ if (action->vport.caps->num == WIRE_PORT)
goto out_invalid_arg;
attr.final_icm_addr = action->vport.caps->icm_address_rx;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_types.h b/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_types.h
index a37ee6359be2..78f899fb3305 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_types.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_types.h
@@ -745,7 +745,6 @@ struct mlx5dr_action {
struct {
struct mlx5dr_domain *dmn;
struct mlx5dr_cmd_vport_cap *caps;
- u32 num;
} vport;
struct {
u32 vlan_hdr; /* tpid_pcp_dei_vid */
--
2.21.0
^ permalink raw reply related
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