* Re: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
From: Simon Horman @ 2019-01-23 9:51 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Wolfgang Grandegger, Marc Kleine-Budde, Rob Herring, Mark Rutland,
Michael Turquette, Stephen Boyd, David S. Miller, Magnus Damm,
Geert Uytterhoeven, Chris Paterson, Biju Das, linux-can, netdev,
devicetree, linux-renesas-soc, linux-clk
In-Reply-To: <20190118121353.3mb44a7rmmp2ru5d@verge.net.au>
On Fri, Jan 18, 2019 at 01:13:53PM +0100, Simon Horman wrote:
> On Thu, Jan 17, 2019 at 02:54:15PM +0000, Fabrizio Castro wrote:
> > According to the latest information, clkp2 is available on RZ/G2.
> > Modify CAN0 and CAN1 nodes accordingly.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
>
> Thanks,
>
> This looks fine to me but I will wait to see if there are other reviews
> before applying.
>
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Thanks again, applied for v5.1.
^ permalink raw reply
* Re: [PATCH net-next] devlink: Use after free in devlink_health_reporter_destroy()
From: Eran Ben Elisha @ 2019-01-23 9:52 UTC (permalink / raw)
To: Dan Carpenter, Jiri Pirko
Cc: David S. Miller, netdev@vger.kernel.org,
kernel-janitors@vger.kernel.org
In-Reply-To: <20190123094434.GB26018@kadam>
On 1/23/2019 11:44 AM, Dan Carpenter wrote:
> This calls kfree(reporter); before dereferencing reporter on the next
> line when it does mutex_unlock(&reporter->devlink->lock);
>
> Fixes: 880ee82f0313 ("devlink: Add health reporter create/destroy functionality")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Hi, thanks for the fix.
It is identical to one I posted yesterday.
https://patchwork.ozlabs.org/patch/1029358/
> ---
> net/core/devlink.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 60248a53c0ad..deccce13285a 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4223,14 +4223,16 @@ EXPORT_SYMBOL_GPL(devlink_health_reporter_create);
> void
> devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
> {
> - mutex_lock(&reporter->devlink->lock);
> + struct devlink *devlink = reporter->devlink;
> +
> + mutex_lock(&devlink->lock);
> list_del(&reporter->list);
> devlink_health_buffers_destroy(reporter->dump_buffers_array,
> DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
> devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
> DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
> kfree(reporter);
> - mutex_unlock(&reporter->devlink->lock);
> + mutex_unlock(&devlink->lock);
> }
> EXPORT_SYMBOL_GPL(devlink_health_reporter_destroy);
>
>
^ permalink raw reply
* [PATCH net-next V4 1/5] vhost: generalize adding used elem
From: Jason Wang @ 2019-01-23 9:55 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel, jasowang; +Cc: mst, kvm
In-Reply-To: <20190123095557.30168-1-jasowang@redhat.com>
Use one generic vhost_copy_to_user() instead of two dedicated
accessor. This will simplify the conversion to fine grain
accessors. About 2% improvement of PPS were seen during vitio-user
txonly test.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 15a216cdd507..14fad2577df3 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2250,16 +2250,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
start = vq->last_used_idx & (vq->num - 1);
used = vq->used->ring + start;
- if (count == 1) {
- if (vhost_put_user(vq, heads[0].id, &used->id)) {
- vq_err(vq, "Failed to write used id");
- return -EFAULT;
- }
- if (vhost_put_user(vq, heads[0].len, &used->len)) {
- vq_err(vq, "Failed to write used len");
- return -EFAULT;
- }
- } else if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
+ if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
vq_err(vq, "Failed to write used");
return -EFAULT;
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next V4 3/5] vhost: rename vq_iotlb_prefetch() to vq_meta_prefetch()
From: Jason Wang @ 2019-01-23 9:55 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel, jasowang; +Cc: mst, kvm
In-Reply-To: <20190123095557.30168-1-jasowang@redhat.com>
Rename the function to be more accurate since it actually tries to
prefetch vq metadata address in IOTLB. And this will be used by
following patch to prefetch metadata virtual addresses.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 4 ++--
drivers/vhost/vhost.c | 4 ++--
drivers/vhost/vhost.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index bca86bf7189f..9c83c1837464 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -971,7 +971,7 @@ static void handle_tx(struct vhost_net *net)
if (!sock)
goto out;
- if (!vq_iotlb_prefetch(vq))
+ if (!vq_meta_prefetch(vq))
goto out;
vhost_disable_notify(&net->dev, vq);
@@ -1140,7 +1140,7 @@ static void handle_rx(struct vhost_net *net)
if (!sock)
goto out;
- if (!vq_iotlb_prefetch(vq))
+ if (!vq_meta_prefetch(vq))
goto out;
vhost_disable_notify(&net->dev, vq);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 96dd87531ba0..24c74c60c093 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1308,7 +1308,7 @@ static bool iotlb_access_ok(struct vhost_virtqueue *vq,
return true;
}
-int vq_iotlb_prefetch(struct vhost_virtqueue *vq)
+int vq_meta_prefetch(struct vhost_virtqueue *vq)
{
size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
unsigned int num = vq->num;
@@ -1327,7 +1327,7 @@ int vq_iotlb_prefetch(struct vhost_virtqueue *vq)
num * sizeof(*vq->used->ring) + s,
VHOST_ADDR_USED);
}
-EXPORT_SYMBOL_GPL(vq_iotlb_prefetch);
+EXPORT_SYMBOL_GPL(vq_meta_prefetch);
/* Can we log writes? */
/* Caller should have device mutex but not vq mutex */
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 1b675dad5e05..4e21011b6628 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -207,7 +207,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len,
struct iovec *iov, int count);
-int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
+int vq_meta_prefetch(struct vhost_virtqueue *vq);
struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
void vhost_enqueue_msg(struct vhost_dev *dev,
--
2.17.1
^ permalink raw reply related
* [PATCH net-next V4 4/5] vhost: introduce helpers to get the size of metadata area
From: Jason Wang @ 2019-01-23 9:55 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel, jasowang; +Cc: mst, kvm
In-Reply-To: <20190123095557.30168-1-jasowang@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 46 ++++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 24c74c60c093..37e2cac8e8b0 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -489,6 +489,27 @@ bool vhost_dev_has_owner(struct vhost_dev *dev)
}
EXPORT_SYMBOL_GPL(vhost_dev_has_owner);
+static size_t vhost_get_avail_size(struct vhost_virtqueue *vq, int num)
+{
+ size_t event = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
+
+ return sizeof(*vq->avail) +
+ sizeof(*vq->avail->ring) * num + event;
+}
+
+static size_t vhost_get_used_size(struct vhost_virtqueue *vq, int num)
+{
+ size_t event = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
+
+ return sizeof(*vq->used) +
+ sizeof(*vq->used->ring) * num + event;
+}
+
+static size_t vhost_get_desc_size(struct vhost_virtqueue *vq, int num)
+{
+ return sizeof(*vq->desc) * num;
+}
+
/* Caller should have device mutex */
long vhost_dev_set_owner(struct vhost_dev *dev)
{
@@ -1252,13 +1273,9 @@ static bool vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
struct vring_used __user *used)
{
- size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
-
- return access_ok(desc, num * sizeof *desc) &&
- access_ok(avail,
- sizeof *avail + num * sizeof *avail->ring + s) &&
- access_ok(used,
- sizeof *used + num * sizeof *used->ring + s);
+ return access_ok(desc, vhost_get_desc_size(vq, num)) &&
+ access_ok(avail, vhost_get_avail_size(vq, num)) &&
+ access_ok(used, vhost_get_used_size(vq, num));
}
static void vhost_vq_meta_update(struct vhost_virtqueue *vq,
@@ -1310,22 +1327,18 @@ static bool iotlb_access_ok(struct vhost_virtqueue *vq,
int vq_meta_prefetch(struct vhost_virtqueue *vq)
{
- size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
unsigned int num = vq->num;
if (!vq->iotlb)
return 1;
return iotlb_access_ok(vq, VHOST_ACCESS_RO, (u64)(uintptr_t)vq->desc,
- num * sizeof(*vq->desc), VHOST_ADDR_DESC) &&
+ vhost_get_desc_size(vq, num), VHOST_ADDR_DESC) &&
iotlb_access_ok(vq, VHOST_ACCESS_RO, (u64)(uintptr_t)vq->avail,
- sizeof *vq->avail +
- num * sizeof(*vq->avail->ring) + s,
+ vhost_get_avail_size(vq, num),
VHOST_ADDR_AVAIL) &&
iotlb_access_ok(vq, VHOST_ACCESS_WO, (u64)(uintptr_t)vq->used,
- sizeof *vq->used +
- num * sizeof(*vq->used->ring) + s,
- VHOST_ADDR_USED);
+ vhost_get_used_size(vq, num), VHOST_ADDR_USED);
}
EXPORT_SYMBOL_GPL(vq_meta_prefetch);
@@ -1342,13 +1355,10 @@ EXPORT_SYMBOL_GPL(vhost_log_access_ok);
static bool vq_log_access_ok(struct vhost_virtqueue *vq,
void __user *log_base)
{
- size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
-
return vq_memory_access_ok(log_base, vq->umem,
vhost_has_feature(vq, VHOST_F_LOG_ALL)) &&
(!vq->log_used || log_access_ok(log_base, vq->log_addr,
- sizeof *vq->used +
- vq->num * sizeof *vq->used->ring + s));
+ vhost_get_used_size(vq, vq->num)));
}
/* Can we start vq? */
--
2.17.1
^ permalink raw reply related
* [PATCH net-next V4 5/5] vhost: access vq metadata through kernel virtual address
From: Jason Wang @ 2019-01-23 9:55 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel, jasowang; +Cc: mst, kvm
In-Reply-To: <20190123095557.30168-1-jasowang@redhat.com>
It was noticed that the copy_user() friends that was used to access
virtqueue metdata tends to be very expensive for dataplane
implementation like vhost since it involves lots of software checks,
speculation barrier, hardware feature toggling (e.g SMAP). The
extra cost will be more obvious when transferring small packets since
the time spent on metadata accessing become more significant.
This patch tries to eliminate those overheads by accessing them
through kernel virtual address by vmap(). To make the pages can be
migrated, instead of pinning them through GUP, we use MMU notifiers to
invalidate vmaps and re-establish vmaps during each round of metadata
prefetching if necessary. For devices that doesn't use metadata
prefetching, the memory accessors fallback to normal copy_user()
implementation gracefully. The invalidation was synchronized with
datapath through vq mutex, and in order to avoid hold vq mutex during
range checking, MMU notifier was teared down when trying to modify vq
metadata.
Another thing is kernel lacks efficient solution for tracking dirty
pages by vmap(), this will lead issues if vhost is using file backed
memory which needs care of writeback. This patch solves this issue by
just skipping the vma that is file backed and fallback to normal
copy_user() friends. This might introduce some overheads for file
backed users but consider this use case is rare we could do
optimizations on top.
Note that this was only done when device IOTLB is not enabled. We
could use similar method to optimize it in the future.
Tests shows at most about 22% improvement on TX PPS when using
virtio-user + vhost_net + xdp1 + TAP on 2.6GHz Broadwell:
SMAP on | SMAP off
Before: 5.0Mpps | 6.6Mpps
After: 6.1Mpps | 7.4Mpps
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 288 +++++++++++++++++++++++++++++++++++++++++-
drivers/vhost/vhost.h | 13 ++
mm/shmem.c | 1 +
3 files changed, 300 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 37e2cac8e8b0..096ae3298d62 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -440,6 +440,9 @@ void vhost_dev_init(struct vhost_dev *dev,
vq->indirect = NULL;
vq->heads = NULL;
vq->dev = dev;
+ memset(&vq->avail_ring, 0, sizeof(vq->avail_ring));
+ memset(&vq->used_ring, 0, sizeof(vq->used_ring));
+ memset(&vq->desc_ring, 0, sizeof(vq->desc_ring));
mutex_init(&vq->mutex);
vhost_vq_reset(dev, vq);
if (vq->handle_kick)
@@ -510,6 +513,73 @@ static size_t vhost_get_desc_size(struct vhost_virtqueue *vq, int num)
return sizeof(*vq->desc) * num;
}
+static void vhost_uninit_vmap(struct vhost_vmap *map)
+{
+ if (map->addr)
+ vunmap(map->unmap_addr);
+
+ map->addr = NULL;
+ map->unmap_addr = NULL;
+}
+
+static int vhost_invalidate_vmap(struct vhost_virtqueue *vq,
+ struct vhost_vmap *map,
+ unsigned long ustart,
+ size_t size,
+ unsigned long start,
+ unsigned long end,
+ bool blockable)
+{
+ if (end < ustart || start > ustart - 1 + size)
+ return 0;
+
+ if (!blockable)
+ return -EAGAIN;
+
+ mutex_lock(&vq->mutex);
+ vhost_uninit_vmap(map);
+ mutex_unlock(&vq->mutex);
+
+ return 0;
+}
+
+static int vhost_invalidate_range_start(struct mmu_notifier *mn,
+ const struct mmu_notifier_range *range)
+{
+ struct vhost_dev *dev = container_of(mn, struct vhost_dev,
+ mmu_notifier);
+ int i;
+
+ for (i = 0; i < dev->nvqs; i++) {
+ struct vhost_virtqueue *vq = dev->vqs[i];
+
+ if (vhost_invalidate_vmap(vq, &vq->avail_ring,
+ (unsigned long)vq->avail,
+ vhost_get_avail_size(vq, vq->num),
+ range->start, range->end,
+ range->blockable))
+ return -EAGAIN;
+ if (vhost_invalidate_vmap(vq, &vq->desc_ring,
+ (unsigned long)vq->desc,
+ vhost_get_desc_size(vq, vq->num),
+ range->start, range->end,
+ range->blockable))
+ return -EAGAIN;
+ if (vhost_invalidate_vmap(vq, &vq->used_ring,
+ (unsigned long)vq->used,
+ vhost_get_used_size(vq, vq->num),
+ range->start, range->end,
+ range->blockable))
+ return -EAGAIN;
+ }
+
+ return 0;
+}
+
+static const struct mmu_notifier_ops vhost_mmu_notifier_ops = {
+ .invalidate_range_start = vhost_invalidate_range_start,
+};
+
/* Caller should have device mutex */
long vhost_dev_set_owner(struct vhost_dev *dev)
{
@@ -541,7 +611,14 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
if (err)
goto err_cgroup;
+ dev->mmu_notifier.ops = &vhost_mmu_notifier_ops;
+ err = mmu_notifier_register(&dev->mmu_notifier, dev->mm);
+ if (err)
+ goto err_mmu_notifier;
+
return 0;
+err_mmu_notifier:
+ vhost_dev_free_iovecs(dev);
err_cgroup:
kthread_stop(worker);
dev->worker = NULL;
@@ -632,6 +709,97 @@ static void vhost_clear_msg(struct vhost_dev *dev)
spin_unlock(&dev->iotlb_lock);
}
+/* Suppress the vma that needs writeback since we can not track dirty
+ * pages now.
+ */
+static bool vma_can_vmap(struct vm_area_struct *vma)
+{
+ return vma_is_anonymous(vma) || is_vm_hugetlb_page(vma) ||
+ vma_is_shmem(vma);
+}
+
+static int vhost_init_vmap(struct vhost_dev *dev,
+ struct vhost_vmap *map, unsigned long uaddr,
+ size_t size, int write)
+{
+ struct mm_struct *mm = dev->mm;
+ struct vm_area_struct *vma;
+ struct page **pages;
+ int npages = DIV_ROUND_UP(size, PAGE_SIZE);
+ int npinned;
+ void *vaddr;
+ int err = 0;
+
+ down_read(&mm->mmap_sem);
+ vma = find_vma(mm, uaddr);
+ if (!vma || !vma_can_vmap(vma) ||
+ vma->vm_end < uaddr - 1 + size) {
+ err = -EINVAL;
+ goto err_vma;
+ }
+
+ pages = kmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
+ if (!pages) {
+ err = -ENOMEM;
+ goto err_alloc;
+ }
+
+ npinned = get_user_pages_fast(uaddr, npages, write, pages);
+ if (npinned != npages) {
+ err = -EFAULT;
+ goto err_gup;
+ }
+
+ vaddr = vmap(pages, npages, VM_MAP, PAGE_KERNEL);
+ if (!vaddr) {
+ err = EFAULT;
+ goto err_gup;
+ }
+
+ map->addr = vaddr + (uaddr & (PAGE_SIZE - 1));
+ map->unmap_addr = vaddr;
+
+err_gup:
+ /* Don't pin pages, mmu notifier will notify us about page
+ * migration.
+ */
+ if (npinned > 0)
+ release_pages(pages, npinned);
+err_alloc:
+ kfree(pages);
+err_vma:
+ up_read(&mm->mmap_sem);
+ return err;
+}
+
+static void vhost_clean_vmaps(struct vhost_virtqueue *vq)
+{
+ vhost_uninit_vmap(&vq->avail_ring);
+ vhost_uninit_vmap(&vq->desc_ring);
+ vhost_uninit_vmap(&vq->used_ring);
+}
+
+static int vhost_setup_avail_vmap(struct vhost_virtqueue *vq,
+ unsigned long avail)
+{
+ return vhost_init_vmap(vq->dev, &vq->avail_ring, avail,
+ vhost_get_avail_size(vq, vq->num), false);
+}
+
+static int vhost_setup_desc_vmap(struct vhost_virtqueue *vq,
+ unsigned long desc)
+{
+ return vhost_init_vmap(vq->dev, &vq->desc_ring, desc,
+ vhost_get_desc_size(vq, vq->num), false);
+}
+
+static int vhost_setup_used_vmap(struct vhost_virtqueue *vq,
+ unsigned long used)
+{
+ return vhost_init_vmap(vq->dev, &vq->used_ring, used,
+ vhost_get_used_size(vq, vq->num), true);
+}
+
void vhost_dev_cleanup(struct vhost_dev *dev)
{
int i;
@@ -661,8 +829,12 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
kthread_stop(dev->worker);
dev->worker = NULL;
}
- if (dev->mm)
+ if (dev->mm) {
+ mmu_notifier_unregister(&dev->mmu_notifier, dev->mm);
mmput(dev->mm);
+ }
+ for (i = 0; i < dev->nvqs; i++)
+ vhost_clean_vmaps(dev->vqs[i]);
dev->mm = NULL;
}
EXPORT_SYMBOL_GPL(vhost_dev_cleanup);
@@ -891,6 +1063,16 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
static inline int vhost_put_avail_event(struct vhost_virtqueue *vq)
{
+ if (!vq->iotlb) {
+ struct vring_used *used = vq->used_ring.addr;
+
+ if (likely(used)) {
+ *((__virtio16 *)&used->ring[vq->num]) =
+ cpu_to_vhost16(vq, vq->avail_idx);
+ return 0;
+ }
+ }
+
return vhost_put_user(vq, cpu_to_vhost16(vq, vq->avail_idx),
vhost_avail_event(vq));
}
@@ -899,6 +1081,16 @@ static inline int vhost_put_used(struct vhost_virtqueue *vq,
struct vring_used_elem *head, int idx,
int count)
{
+ if (!vq->iotlb) {
+ struct vring_used *used = vq->used_ring.addr;
+
+ if (likely(used)) {
+ memcpy(used->ring + idx, head,
+ count * sizeof(*head));
+ return 0;
+ }
+ }
+
return vhost_copy_to_user(vq, vq->used->ring + idx, head,
count * sizeof(*head));
}
@@ -906,6 +1098,15 @@ static inline int vhost_put_used(struct vhost_virtqueue *vq,
static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
{
+ if (!vq->iotlb) {
+ struct vring_used *used = vq->used_ring.addr;
+
+ if (likely(used)) {
+ used->flags = cpu_to_vhost16(vq, vq->used_flags);
+ return 0;
+ }
+ }
+
return vhost_put_user(vq, cpu_to_vhost16(vq, vq->used_flags),
&vq->used->flags);
}
@@ -913,6 +1114,15 @@ static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
static inline int vhost_put_used_idx(struct vhost_virtqueue *vq)
{
+ if (!vq->iotlb) {
+ struct vring_used *used = vq->used_ring.addr;
+
+ if (likely(used)) {
+ used->idx = cpu_to_vhost16(vq, vq->last_used_idx);
+ return 0;
+ }
+ }
+
return vhost_put_user(vq, cpu_to_vhost16(vq, vq->last_used_idx),
&vq->used->idx);
}
@@ -958,12 +1168,30 @@ static void vhost_dev_unlock_vqs(struct vhost_dev *d)
static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq,
__virtio16 *idx)
{
+ if (!vq->iotlb) {
+ struct vring_avail *avail = vq->avail_ring.addr;
+
+ if (likely(avail)) {
+ *idx = avail->idx;
+ return 0;
+ }
+ }
+
return vhost_get_avail(vq, *idx, &vq->avail->idx);
}
static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
__virtio16 *head, int idx)
{
+ if (!vq->iotlb) {
+ struct vring_avail *avail = vq->avail_ring.addr;
+
+ if (likely(avail)) {
+ *head = avail->ring[idx & (vq->num - 1)];
+ return 0;
+ }
+ }
+
return vhost_get_avail(vq, *head,
&vq->avail->ring[idx & (vq->num - 1)]);
}
@@ -971,24 +1199,60 @@ static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
static inline int vhost_get_avail_flags(struct vhost_virtqueue *vq,
__virtio16 *flags)
{
+ if (!vq->iotlb) {
+ struct vring_avail *avail = vq->avail_ring.addr;
+
+ if (likely(avail)) {
+ *flags = avail->flags;
+ return 0;
+ }
+ }
+
return vhost_get_avail(vq, *flags, &vq->avail->flags);
}
static inline int vhost_get_used_event(struct vhost_virtqueue *vq,
__virtio16 *event)
{
+ if (!vq->iotlb) {
+ struct vring_avail *avail = vq->avail_ring.addr;
+
+ if (likely(avail)) {
+ *event = (__virtio16)avail->ring[vq->num];
+ return 0;
+ }
+ }
+
return vhost_get_avail(vq, *event, vhost_used_event(vq));
}
static inline int vhost_get_used_idx(struct vhost_virtqueue *vq,
__virtio16 *idx)
{
+ if (!vq->iotlb) {
+ struct vring_used *used = vq->used_ring.addr;
+
+ if (likely(used)) {
+ *idx = used->idx;
+ return 0;
+ }
+ }
+
return vhost_get_used(vq, *idx, &vq->used->idx);
}
static inline int vhost_get_desc(struct vhost_virtqueue *vq,
struct vring_desc *desc, int idx)
{
+ if (!vq->iotlb) {
+ struct vring_desc *d = vq->desc_ring.addr;
+
+ if (likely(d)) {
+ *desc = *(d + idx);
+ return 0;
+ }
+ }
+
return vhost_copy_from_user(vq, desc, vq->desc + idx, sizeof(*desc));
}
@@ -1329,8 +1593,16 @@ int vq_meta_prefetch(struct vhost_virtqueue *vq)
{
unsigned int num = vq->num;
- if (!vq->iotlb)
+ if (!vq->iotlb) {
+ if (unlikely(!vq->avail_ring.addr))
+ vhost_setup_avail_vmap(vq, (unsigned long)vq->avail);
+ if (unlikely(!vq->desc_ring.addr))
+ vhost_setup_desc_vmap(vq, (unsigned long)vq->desc);
+ if (unlikely(!vq->used_ring.addr))
+ vhost_setup_used_vmap(vq, (unsigned long)vq->used);
+
return 1;
+ }
return iotlb_access_ok(vq, VHOST_ACCESS_RO, (u64)(uintptr_t)vq->desc,
vhost_get_desc_size(vq, num), VHOST_ADDR_DESC) &&
@@ -1482,6 +1754,13 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
mutex_lock(&vq->mutex);
+ /* Unregister MMU notifer to allow invalidation callback
+ * can access vq->avail, vq->desc , vq->used and vq->num
+ * without holding vq->mutex.
+ */
+ if (d->mm)
+ mmu_notifier_unregister(&d->mmu_notifier, d->mm);
+
switch (ioctl) {
case VHOST_SET_VRING_NUM:
/* Resizing ring with an active backend?
@@ -1498,6 +1777,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
r = -EINVAL;
break;
}
+ vhost_clean_vmaps(vq);
vq->num = s.num;
break;
case VHOST_SET_VRING_BASE:
@@ -1575,6 +1855,8 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
}
}
+ vhost_clean_vmaps(vq);
+
vq->log_used = !!(a.flags & (0x1 << VHOST_VRING_F_LOG));
vq->desc = (void __user *)(unsigned long)a.desc_user_addr;
vq->avail = (void __user *)(unsigned long)a.avail_user_addr;
@@ -1655,6 +1937,8 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
if (pollstart && vq->handle_kick)
r = vhost_poll_start(&vq->poll, vq->kick);
+ if (d->mm)
+ mmu_notifier_register(&d->mmu_notifier, d->mm);
mutex_unlock(&vq->mutex);
if (pollstop && vq->handle_kick)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 4e21011b6628..c04bc327db9f 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -12,6 +12,8 @@
#include <linux/virtio_config.h>
#include <linux/virtio_ring.h>
#include <linux/atomic.h>
+#include <linux/pagemap.h>
+#include <linux/mmu_notifier.h>
struct vhost_work;
typedef void (*vhost_work_fn_t)(struct vhost_work *work);
@@ -80,6 +82,11 @@ enum vhost_uaddr_type {
VHOST_NUM_ADDRS = 3,
};
+struct vhost_vmap {
+ void *addr;
+ void *unmap_addr;
+};
+
/* The virtqueue structure describes a queue attached to a device. */
struct vhost_virtqueue {
struct vhost_dev *dev;
@@ -90,6 +97,11 @@ struct vhost_virtqueue {
struct vring_desc __user *desc;
struct vring_avail __user *avail;
struct vring_used __user *used;
+
+ struct vhost_vmap avail_ring;
+ struct vhost_vmap desc_ring;
+ struct vhost_vmap used_ring;
+
const struct vhost_umem_node *meta_iotlb[VHOST_NUM_ADDRS];
struct file *kick;
struct eventfd_ctx *call_ctx;
@@ -158,6 +170,7 @@ struct vhost_msg_node {
struct vhost_dev {
struct mm_struct *mm;
+ struct mmu_notifier mmu_notifier;
struct mutex mutex;
struct vhost_virtqueue **vqs;
int nvqs;
diff --git a/mm/shmem.c b/mm/shmem.c
index 6ece1e2fe76e..745e7c7f7a6c 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -237,6 +237,7 @@ bool vma_is_shmem(struct vm_area_struct *vma)
{
return vma->vm_ops == &shmem_vm_ops;
}
+EXPORT_SYMBOL_GPL(vma_is_shmem);
static LIST_HEAD(shmem_swaplist);
static DEFINE_MUTEX(shmem_swaplist_mutex);
--
2.17.1
^ permalink raw reply related
* [PATCH net-next V4 2/5] vhost: fine grain userspace memory accessors
From: Jason Wang @ 2019-01-23 9:55 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel, jasowang; +Cc: mst, kvm
In-Reply-To: <20190123095557.30168-1-jasowang@redhat.com>
This is used to hide the metadata address from virtqueue helpers. This
will allow to implement a vmap based fast accessing to metadata.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 94 +++++++++++++++++++++++++++++++++++--------
1 file changed, 77 insertions(+), 17 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 14fad2577df3..96dd87531ba0 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -868,6 +868,34 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
ret; \
})
+static inline int vhost_put_avail_event(struct vhost_virtqueue *vq)
+{
+ return vhost_put_user(vq, cpu_to_vhost16(vq, vq->avail_idx),
+ vhost_avail_event(vq));
+}
+
+static inline int vhost_put_used(struct vhost_virtqueue *vq,
+ struct vring_used_elem *head, int idx,
+ int count)
+{
+ return vhost_copy_to_user(vq, vq->used->ring + idx, head,
+ count * sizeof(*head));
+}
+
+static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
+
+{
+ return vhost_put_user(vq, cpu_to_vhost16(vq, vq->used_flags),
+ &vq->used->flags);
+}
+
+static inline int vhost_put_used_idx(struct vhost_virtqueue *vq)
+
+{
+ return vhost_put_user(vq, cpu_to_vhost16(vq, vq->last_used_idx),
+ &vq->used->idx);
+}
+
#define vhost_get_user(vq, x, ptr, type) \
({ \
int ret; \
@@ -906,6 +934,43 @@ static void vhost_dev_unlock_vqs(struct vhost_dev *d)
mutex_unlock(&d->vqs[i]->mutex);
}
+static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq,
+ __virtio16 *idx)
+{
+ return vhost_get_avail(vq, *idx, &vq->avail->idx);
+}
+
+static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
+ __virtio16 *head, int idx)
+{
+ return vhost_get_avail(vq, *head,
+ &vq->avail->ring[idx & (vq->num - 1)]);
+}
+
+static inline int vhost_get_avail_flags(struct vhost_virtqueue *vq,
+ __virtio16 *flags)
+{
+ return vhost_get_avail(vq, *flags, &vq->avail->flags);
+}
+
+static inline int vhost_get_used_event(struct vhost_virtqueue *vq,
+ __virtio16 *event)
+{
+ return vhost_get_avail(vq, *event, vhost_used_event(vq));
+}
+
+static inline int vhost_get_used_idx(struct vhost_virtqueue *vq,
+ __virtio16 *idx)
+{
+ return vhost_get_used(vq, *idx, &vq->used->idx);
+}
+
+static inline int vhost_get_desc(struct vhost_virtqueue *vq,
+ struct vring_desc *desc, int idx)
+{
+ return vhost_copy_from_user(vq, desc, vq->desc + idx, sizeof(*desc));
+}
+
static int vhost_new_umem_range(struct vhost_umem *umem,
u64 start, u64 size, u64 end,
u64 userspace_addr, int perm)
@@ -1839,8 +1904,7 @@ EXPORT_SYMBOL_GPL(vhost_log_write);
static int vhost_update_used_flags(struct vhost_virtqueue *vq)
{
void __user *used;
- if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->used_flags),
- &vq->used->flags) < 0)
+ if (vhost_put_used_flags(vq))
return -EFAULT;
if (unlikely(vq->log_used)) {
/* Make sure the flag is seen before log. */
@@ -1857,8 +1921,7 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
{
- if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->avail_idx),
- vhost_avail_event(vq)))
+ if (vhost_put_avail_event(vq))
return -EFAULT;
if (unlikely(vq->log_used)) {
void __user *used;
@@ -1894,7 +1957,7 @@ int vhost_vq_init_access(struct vhost_virtqueue *vq)
r = -EFAULT;
goto err;
}
- r = vhost_get_used(vq, last_used_idx, &vq->used->idx);
+ r = vhost_get_used_idx(vq, &last_used_idx);
if (r) {
vq_err(vq, "Can't access used idx at %p\n",
&vq->used->idx);
@@ -2093,7 +2156,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
last_avail_idx = vq->last_avail_idx;
if (vq->avail_idx == vq->last_avail_idx) {
- if (unlikely(vhost_get_avail(vq, avail_idx, &vq->avail->idx))) {
+ if (unlikely(vhost_get_avail_idx(vq, &avail_idx))) {
vq_err(vq, "Failed to access avail idx at %p\n",
&vq->avail->idx);
return -EFAULT;
@@ -2120,8 +2183,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
/* Grab the next descriptor number they're advertising, and increment
* the index we've seen. */
- if (unlikely(vhost_get_avail(vq, ring_head,
- &vq->avail->ring[last_avail_idx & (vq->num - 1)]))) {
+ if (unlikely(vhost_get_avail_head(vq, &ring_head, last_avail_idx))) {
vq_err(vq, "Failed to read head: idx %d address %p\n",
last_avail_idx,
&vq->avail->ring[last_avail_idx % vq->num]);
@@ -2156,8 +2218,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
i, vq->num, head);
return -EINVAL;
}
- ret = vhost_copy_from_user(vq, &desc, vq->desc + i,
- sizeof desc);
+ ret = vhost_get_desc(vq, &desc, i);
if (unlikely(ret)) {
vq_err(vq, "Failed to get descriptor: idx %d addr %p\n",
i, vq->desc + i);
@@ -2250,7 +2311,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
start = vq->last_used_idx & (vq->num - 1);
used = vq->used->ring + start;
- if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
+ if (vhost_put_used(vq, heads, start, count)) {
vq_err(vq, "Failed to write used");
return -EFAULT;
}
@@ -2292,8 +2353,7 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
/* Make sure buffer is written before we update index. */
smp_wmb();
- if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->last_used_idx),
- &vq->used->idx)) {
+ if (vhost_put_used_idx(vq)) {
vq_err(vq, "Failed to increment used idx");
return -EFAULT;
}
@@ -2326,7 +2386,7 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (!vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX)) {
__virtio16 flags;
- if (vhost_get_avail(vq, flags, &vq->avail->flags)) {
+ if (vhost_get_avail_flags(vq, &flags)) {
vq_err(vq, "Failed to get flags");
return true;
}
@@ -2340,7 +2400,7 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (unlikely(!v))
return true;
- if (vhost_get_avail(vq, event, vhost_used_event(vq))) {
+ if (vhost_get_used_event(vq, &event)) {
vq_err(vq, "Failed to get used event idx");
return true;
}
@@ -2385,7 +2445,7 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (vq->avail_idx != vq->last_avail_idx)
return false;
- r = vhost_get_avail(vq, avail_idx, &vq->avail->idx);
+ r = vhost_get_avail_idx(vq, &avail_idx);
if (unlikely(r))
return false;
vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
@@ -2421,7 +2481,7 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
/* They could have slipped one in as we were doing that: make
* sure it's written, then check again. */
smp_mb();
- r = vhost_get_avail(vq, avail_idx, &vq->avail->idx);
+ r = vhost_get_avail_idx(vq, &avail_idx);
if (r) {
vq_err(vq, "Failed to check avail idx at %p: %d\n",
&vq->avail->idx, r);
--
2.17.1
^ permalink raw reply related
* [PATCH net-next V4 0/5] vhost: accelerate metadata access through vmap()
From: Jason Wang @ 2019-01-23 9:55 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel, jasowang; +Cc: mst, kvm
This series tries to access virtqueue metadata through kernel virtual
address instead of copy_user() friends since they had too much
overheads like checks, spec barriers or even hardware feature
toggling.
Test shows about 24% improvement on TX PPS. It should benefit other
cases as well.
Changes from V3:
- don't try to use vmap for file backed pages
- rebase to master
Changes from V2:
- fix buggy range overlapping check
- tear down MMU notifier during vhost ioctl to make sure invalidation
request can read metadata userspace address and vq size without
holding vq mutex.
Changes from V1:
- instead of pinning pages, use MMU notifier to invalidate vmaps and
remap duing metadata prefetch
- fix build warning on MIPS
Jason Wang (5):
vhost: generalize adding used elem
vhost: fine grain userspace memory accessors
vhost: rename vq_iotlb_prefetch() to vq_meta_prefetch()
vhost: introduce helpers to get the size of metadata area
vhost: access vq metadata through kernel virtual address
drivers/vhost/net.c | 4 +-
drivers/vhost/vhost.c | 441 +++++++++++++++++++++++++++++++++++++-----
drivers/vhost/vhost.h | 15 +-
mm/shmem.c | 1 +
4 files changed, 410 insertions(+), 51 deletions(-)
--
2.17.1
^ permalink raw reply
* RE: [PATCH 1/7] net: Don't set transport offset to invalid value
From: Maxim Mikityanskiy @ 2019-01-23 10:16 UTC (permalink / raw)
To: Willem de Bruijn
Cc: David S. Miller, Saeed Mahameed, Willem de Bruijn, Jason Wang,
Eric Dumazet, netdev@vger.kernel.org, Eran Ben Elisha,
Tariq Toukan
In-Reply-To: <CAF=yD-J3mG2oV2Z_1QA90G-z1snM_qF3tp5BbP=0Y7V5TGNtkQ@mail.gmail.com>
> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Sent: 17 January, 2019 17:16
> To: Maxim Mikityanskiy <maximmi@mellanox.com>
> Cc: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>;
> netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> <tariqt@mellanox.com>
> Subject: Re: [PATCH 1/7] net: Don't set transport offset to invalid value
>
> On Thu, Jan 17, 2019 at 4:10 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> wrote:
> >
> > > This is a lot of code change. This would do.
> > >
> > > @@ -2434,8 +2434,6 @@ static inline void
> > > skb_probe_transport_header(struct sk_buff *skb,
> > >
> > > if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0,
> 0))
> > > skb_set_transport_header(skb, keys.control.thoff);
> > > - else
> > > - skb_set_transport_header(skb, offset_hint);
> > > }
> > >
> > > Though leaving an unused argument is a bit ugly. For net-next, indeed
> > > better to clean up (please mark your patchset with net or net-next,
> > > btw)
> >
> > It's for net-next (I'll resend with the correct mark), so I'll stick
> > with the current implementation.
>
> Absolutely, sounds good.
>
> > > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
> > > netback/netback.c
> > > > index 80aae3a32c2a..b49b6e56ca47 100644
> > > > --- a/drivers/net/xen-netback/netback.c
> > > > +++ b/drivers/net/xen-netback/netback.c
> > > > @@ -1105,6 +1105,7 @@ static int xenvif_tx_submit(struct xenvif_queue
> > > *queue)
> > > > struct xen_netif_tx_request *txp;
> > > > u16 pending_idx;
> > > > unsigned data_len;
> > > > + bool th_set;
> > > >
> > > > pending_idx = XENVIF_TX_CB(skb)->pending_idx;
> > > > txp = &queue->pending_tx_info[pending_idx].req;
> > > > @@ -1169,20 +1170,22 @@ static int xenvif_tx_submit(struct
> xenvif_queue
> > > *queue)
> > > > continue;
> > > > }
> > > >
> > > > - skb_probe_transport_header(skb, 0);
> > > > + th_set = skb_try_probe_transport_header(skb);
> > >
> > > Can use skb_transport_header_was_set(). Then at least there is no need
> > > to change the function's return value.
> >
> > I suppose this comment relates to the previous one, and if we do it for
> > net-next, it's fine to make change I made, isn't it?
>
> If this is the only reason for the boolean return value, using
> skb_transport_header_was_set() is more standard (I immediately know
> what's happening when I read it), slightly less code change and avoids
> introducing a situation where the majority of callers ignore a return
> value. I think it's preferable. But these merits are certainly
> debatable, so either is fine.
From my side, I wanted to avoid calling skb_transport_header_was_set
twice, so I made skb_try_probe_transport_header return whether it
succeeded or not. I think "try" in the function name indicates this idea
pretty clearly. This result status is pretty useful, it just happened
that it's not needed in many places, but the general idea is that we
report this status, so if you say that my version is also good for you,
I'll leave it as is. It was just a rationale for my decision.
^ permalink raw reply
* RE: [PATCH 0/7] AF_PACKET transport_offset fix
From: Maxim Mikityanskiy @ 2019-01-23 10:16 UTC (permalink / raw)
To: Willem de Bruijn, David S. Miller
Cc: netdev@vger.kernel.org, Saeed Mahameed, Eran Ben Elisha,
Tariq Toukan, Jason Wang, Eric Dumazet
In-Reply-To: <20190114131841.1932-1-maximmi@mellanox.com>
From my perspective, after the discussion we had with Willem, the
current version of this series can be merged to net-next.
Willem, do you approve it?
Thanks for reviewing!
> -----Original Message-----
> From: Maxim Mikityanskiy
> Sent: 14 January, 2019 15:19
> To: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>
> Cc: netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq
> Toukan <tariqt@mellanox.com>; Maxim Mikityanskiy <maximmi@mellanox.com>
> Subject: [PATCH 0/7] AF_PACKET transport_offset fix
>
> This patch series contains the implementation of the RFC that was posted
> on this mailing list previously:
> https://www.spinics.net/lists/netdev/msg541709.html
>
> It fixes having incorrect skb->transport_header values in cases when
> dissect fails. Having correct values set by the kernel fixes mlx5
> operation and allows to remove some unnecessary code flows in mlx5.
>
> Maxim Mikityanskiy (7):
> net: Don't set transport offset to invalid value
> net: Introduce parse_protocol header_ops callback
> net/ethernet: Add parse_protocol header_ops support
> net/packet: Ask driver for protocol if not provided by user
> net/packet: Remove redundant skb->protocol set
> net/mlx5e: Remove the wrong assumption about transport offset
> net/mlx5e: Trust kernel regarding transport offset
>
> .../net/ethernet/mellanox/mlx5/core/en_tx.c | 15 ++---------
> drivers/net/tap.c | 4 +--
> drivers/net/tun.c | 4 +--
> drivers/net/xen-netback/netback.c | 19 ++++++++------
> include/linux/etherdevice.h | 1 +
> include/linux/netdevice.h | 10 +++++++
> include/linux/skbuff.h | 14 +++++-----
> net/ethernet/eth.c | 13 ++++++++++
> net/packet/af_packet.c | 26 +++++++++----------
> 9 files changed, 60 insertions(+), 46 deletions(-)
>
> --
> 2.19.1
^ permalink raw reply
* Re: [PATCH net-next] net: stmmac: Fix return value check in qcom_ethqos_probe()
From: Niklas Cassel @ 2019-01-23 10:39 UTC (permalink / raw)
To: Wei Yongjun
Cc: Vinod Koul, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
Maxime Coquelin, netdev, linux-stm32, linux-arm-kernel,
kernel-janitors
In-Reply-To: <1548224358-83281-1-git-send-email-weiyongjun1@huawei.com>
On Wed, Jan 23, 2019 at 06:19:18AM +0000, Wei Yongjun wrote:
> In case of error, the function devm_clk_get() returns ERR_PTR() and
> never returns NULL. The NULL test in the return value check should be
> replaced with IS_ERR().
>
> Fixes: a7c30e62d4b8 ("net: stmmac: Add driver for Qualcomm ethqos")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 30724bd..7ec8954 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -473,8 +473,8 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
> ethqos->por = of_device_get_match_data(&pdev->dev);
>
> ethqos->rgmii_clk = devm_clk_get(&pdev->dev, "rgmii");
> - if (!ethqos->rgmii_clk) {
> - ret = -ENOMEM;
> + if (IS_ERR(ethqos->rgmii_clk)) {
> + ret = PTR_ERR(ethqos->rgmii_clk);
> goto err_mem;
> }
>
>
>
Acked-by: Niklas Cassel <niklas.cassel@linaro.org>
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/8] libbpf: Add a helper for retrieving a prog via index
From: Daniel Borkmann @ 2019-01-23 10:41 UTC (permalink / raw)
To: Maciej Fijalkowski, ast; +Cc: netdev, jakub.kicinski, brouer
In-Reply-To: <20190121091041.14666-3-maciejromanfijalkowski@gmail.com>
On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
> xdp_redirect_cpu has a 6 different XDP programs that can be attached to
> network interface. This sample has a option --prognum that allows user
> for specifying which particular program from a given set will be
> attached to network interface.
> In order to make it easier when converting the mentioned sample to
> libbpf usage, add a function to libbpf that will return program's fd for
> a given index.
>
> Note that there is already a bpf_object__find_prog_by_idx, which could
> be exported and might be used for that purpose, but it operates on the
> number of ELF section and here we need an index from a programs array
> within the bpf_object.
Series in general looks good to me. Few minor comments, mainly in relation
to the need for libbpf extensions.
Would it not be a better interface to the user to instead choose the prog
based on section name and then retrieve it via bpf_object__find_program_by_title()
instead of prognum (which feels less user friendly) at least?
> Signed-off-by: Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> tools/lib/bpf/libbpf.c | 8 ++++++++
> tools/lib/bpf/libbpf.h | 3 +++
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 12 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index dc838bea403f..21c84d0f6128 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -935,6 +935,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> return err;
> }
>
> +int
> +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx)
> +{
> + if (idx >= 0 && idx < obj->nr_programs)
> + return bpf_program__fd(&obj->programs[idx]);
> + return -ENOENT;
> +}
> +
> static struct bpf_program *
> bpf_object__find_prog_by_idx(struct bpf_object *obj, int idx)
> {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 7f10d36abdde..ca1b381cb3ad 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -95,6 +95,9 @@ LIBBPF_API int bpf_object__btf_fd(const struct bpf_object *obj);
> LIBBPF_API struct bpf_program *
> bpf_object__find_program_by_title(struct bpf_object *obj, const char *title);
>
> +LIBBPF_API int
> +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx);
> +
> LIBBPF_API struct bpf_object *bpf_object__next(struct bpf_object *prev);
> #define bpf_object__for_each_safe(pos, tmp) \
> for ((pos) = bpf_object__next(NULL), \
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 7c59e4f64082..871d2fc07150 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -127,4 +127,5 @@ LIBBPF_0.0.1 {
> LIBBPF_0.0.2 {
> global:
> bpf_object__find_map_fd_by_name;
> + bpf_object__get_prog_fd_by_num;
> } LIBBPF_0.0.1;
>
^ permalink raw reply
* Re: [PATCH bpf-next v2 1/8] libbpf: Add a helper for retrieving a map fd for a given name
From: Daniel Borkmann @ 2019-01-23 10:54 UTC (permalink / raw)
To: Maciej Fijalkowski, ast; +Cc: netdev, jakub.kicinski, brouer
In-Reply-To: <20190121091041.14666-2-maciejromanfijalkowski@gmail.com>
On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
> XDP samples are mostly cooperating with eBPF maps through their file
> descriptors. In case of a eBPF program that contains multiple maps it
> might be tiresome to iterate through them and call bpf_map__fd for each
> one. Add a helper mostly based on bpf_object__find_map_by_name, but
> instead of returning the struct bpf_map pointer, return map fd.
>
> Bump libbpf ABI version to 0.0.2.
>
> Suggested-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> tools/lib/bpf/libbpf.c | 12 ++++++++++++
> tools/lib/bpf/libbpf.h | 3 +++
> tools/lib/bpf/libbpf.map | 4 ++++
> 3 files changed, 19 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 169e347c76f6..dc838bea403f 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2840,6 +2840,18 @@ bpf_object__find_map_by_name(struct bpf_object *obj, const char *name)
> return NULL;
> }
Application could just do: bpf_map__fd(bpf_object__find_map_by_name(...)) or
bpf_object__find_map_by_name(...)->fd as both are exposed via library, though
I guess it may be okay to have a helper for it as it feels this might be needed
in many cases.
> +int
> +bpf_object__find_map_fd_by_name(struct bpf_object *obj, const char *name)
> +{
> + struct bpf_map *pos;
> +
> + bpf_map__for_each(pos, obj) {
> + if (pos->name && !strcmp(pos->name, name))
> + return bpf_map__fd(pos);
> + }
> + return -ENOENT;
Can we instead just do:
int
bpf_object__find_map_fd_by_name(struct bpf_object *obj, const char *name)
{
return bpf_map__fd(bpf_object__find_map_by_name(obj, name));
}
> +}
> +
> struct bpf_map *
> bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset)
> {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 5f68d7b75215..7f10d36abdde 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -264,6 +264,9 @@ struct bpf_map;
> LIBBPF_API struct bpf_map *
> bpf_object__find_map_by_name(struct bpf_object *obj, const char *name);
>
> +LIBBPF_API int
> +bpf_object__find_map_fd_by_name(struct bpf_object *obj, const char *name);
> +
> /*
> * Get bpf_map through the offset of corresponding struct bpf_map_def
> * in the BPF object file.
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index cd02cd4e2cc3..7c59e4f64082 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -124,3 +124,7 @@ LIBBPF_0.0.1 {
> local:
> *;
> };
> +LIBBPF_0.0.2 {
> + global:
> + bpf_object__find_map_fd_by_name;
> +} LIBBPF_0.0.1;
>
^ permalink raw reply
* [PATCH 2/3] gcc-plugins: Introduce stackinit plugin
From: Kees Cook @ 2019-01-23 11:03 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Ard Biesheuvel, Laura Abbott, Alexander Popov,
xen-devel, dri-devel, intel-gfx, intel-wired-lan, netdev,
linux-usb, linux-fsdevel, linux-mm, dev, linux-kbuild,
linux-security-module, kernel-hardening
In-Reply-To: <20190123110349.35882-1-keescook@chromium.org>
This attempts to duplicate the proposed gcc option -finit-local-vars[1]
in an effort to implement the "always initialize local variables" kernel
development goal[2].
Enabling CONFIG_GCC_PLUGIN_STACKINIT should stop all "uninitialized
stack variable" flaws as long as they don't depend on being zero. :)
[1] https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html
[2] https://lkml.kernel.org/r/CA+55aFykZL+cSBJjBBts7ebEFfyGPdMzTmLSxKnT_29=j942dA@mail.gmail.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
scripts/Makefile.gcc-plugins | 6 ++
scripts/gcc-plugins/Kconfig | 9 +++
scripts/gcc-plugins/gcc-common.h | 11 +++-
scripts/gcc-plugins/stackinit_plugin.c | 79 ++++++++++++++++++++++++++
4 files changed, 102 insertions(+), 3 deletions(-)
create mode 100644 scripts/gcc-plugins/stackinit_plugin.c
diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index 35042d96cf5d..2483121d781c 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -12,6 +12,12 @@ export DISABLE_LATENT_ENTROPY_PLUGIN
gcc-plugin-$(CONFIG_GCC_PLUGIN_SANCOV) += sancov_plugin.so
+gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKINIT) += stackinit_plugin.so
+ifdef CONFIG_GCC_PLUGIN_STACKINIT
+ DISABLE_STACKINIT_PLUGIN += -fplugin-arg-stackinit_plugin-disable
+endif
+export DISABLE_STACKINIT_PLUGIN
+
gcc-plugin-$(CONFIG_GCC_PLUGIN_STRUCTLEAK) += structleak_plugin.so
gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK_VERBOSE) \
+= -fplugin-arg-structleak_plugin-verbose
diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
index d45f7f36b859..b117fe83f1d3 100644
--- a/scripts/gcc-plugins/Kconfig
+++ b/scripts/gcc-plugins/Kconfig
@@ -66,6 +66,15 @@ config GCC_PLUGIN_LATENT_ENTROPY
* https://grsecurity.net/
* https://pax.grsecurity.net/
+config GCC_PLUGIN_STACKINIT
+ bool "Initialize all stack variables to zero by default"
+ depends on GCC_PLUGINS
+ depends on !GCC_PLUGIN_STRUCTLEAK
+ help
+ This plugin zero-initializes all stack variables. This is more
+ comprehensive than GCC_PLUGIN_STRUCTLEAK, and attempts to
+ duplicate the proposed -finit-local-vars gcc build flag.
+
config GCC_PLUGIN_STRUCTLEAK
bool "Force initialization of variables containing userspace addresses"
# Currently STRUCTLEAK inserts initialization out of live scope of
diff --git a/scripts/gcc-plugins/gcc-common.h b/scripts/gcc-plugins/gcc-common.h
index 552d5efd7cb7..f690b4deeabd 100644
--- a/scripts/gcc-plugins/gcc-common.h
+++ b/scripts/gcc-plugins/gcc-common.h
@@ -76,6 +76,14 @@
#include "c-common.h"
#endif
+#if BUILDING_GCC_VERSION > 4005
+#include "c-tree.h"
+#else
+/* should come from c-tree.h if only it were installed for gcc 4.5... */
+#define C_TYPE_FIELDS_READONLY(TYPE) TREE_LANG_FLAG_1(TYPE)
+extern bool global_bindings_p (void);
+#endif
+
#if BUILDING_GCC_VERSION <= 4008
#include "tree-flow.h"
#else
@@ -158,9 +166,6 @@ void dump_gimple_stmt(pretty_printer *, gimple, int, int);
#define TYPE_NAME_POINTER(node) IDENTIFIER_POINTER(TYPE_NAME(node))
#define TYPE_NAME_LENGTH(node) IDENTIFIER_LENGTH(TYPE_NAME(node))
-/* should come from c-tree.h if only it were installed for gcc 4.5... */
-#define C_TYPE_FIELDS_READONLY(TYPE) TREE_LANG_FLAG_1(TYPE)
-
static inline tree build_const_char_string(int len, const char *str)
{
tree cstr, elem, index, type;
diff --git a/scripts/gcc-plugins/stackinit_plugin.c b/scripts/gcc-plugins/stackinit_plugin.c
new file mode 100644
index 000000000000..41055cd7098e
--- /dev/null
+++ b/scripts/gcc-plugins/stackinit_plugin.c
@@ -0,0 +1,79 @@
+/* SPDX-License: GPLv2 */
+/*
+ * This will zero-initialize local stack variables. (Though structure
+ * padding may remain uninitialized in certain cases.)
+ *
+ * Implements Florian Weimer's "-finit-local-vars" gcc patch as a plugin:
+ * https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html
+ *
+ * Plugin skeleton code thanks to PaX Team.
+ *
+ * Options:
+ * -fplugin-arg-stackinit_plugin-disable
+ */
+
+#include "gcc-common.h"
+
+__visible int plugin_is_GPL_compatible;
+
+static struct plugin_info stackinit_plugin_info = {
+ .version = "20190122",
+ .help = "disable\tdo not activate plugin\n",
+};
+
+static void finish_decl(void *event_data, void *data)
+{
+ tree decl = (tree)event_data;
+ tree type;
+
+ if (TREE_CODE (decl) != VAR_DECL)
+ return;
+
+ if (DECL_EXTERNAL (decl))
+ return;
+
+ if (DECL_INITIAL (decl) != NULL_TREE)
+ return;
+
+ if (global_bindings_p ())
+ return;
+
+ type = TREE_TYPE (decl);
+ if (AGGREGATE_TYPE_P (type))
+ DECL_INITIAL (decl) = build_constructor (type, NULL);
+ else
+ DECL_INITIAL (decl) = fold_convert (type, integer_zero_node);
+}
+
+__visible int plugin_init(struct plugin_name_args *plugin_info, struct plugin_gcc_version *version)
+{
+ int i;
+ const char * const plugin_name = plugin_info->base_name;
+ const int argc = plugin_info->argc;
+ const struct plugin_argument * const argv = plugin_info->argv;
+ bool enable = true;
+
+ if (!plugin_default_version_check(version, &gcc_version)) {
+ error(G_("incompatible gcc/plugin versions"));
+ return 1;
+ }
+
+ if (strncmp(lang_hooks.name, "GNU C", 5) && !strncmp(lang_hooks.name, "GNU C+", 6)) {
+ inform(UNKNOWN_LOCATION, G_("%s supports C only, not %s"), plugin_name, lang_hooks.name);
+ enable = false;
+ }
+
+ for (i = 0; i < argc; ++i) {
+ if (!strcmp(argv[i].key, "disable")) {
+ enable = false;
+ continue;
+ }
+ error(G_("unknown option '-fplugin-arg-%s-%s'"), plugin_name, argv[i].key);
+ }
+
+ register_callback(plugin_name, PLUGIN_INFO, NULL, &stackinit_plugin_info);
+ if (enable)
+ register_callback(plugin_name, PLUGIN_FINISH_DECL, finish_decl, NULL);
+
+ return 0;
+}
--
2.17.1
^ permalink raw reply related
* [PATCH 1/3] treewide: Lift switch variables out of switches
From: Kees Cook @ 2019-01-23 11:03 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Ard Biesheuvel, Laura Abbott, Alexander Popov,
xen-devel, dri-devel, intel-gfx, intel-wired-lan, netdev,
linux-usb, linux-fsdevel, linux-mm, dev, linux-kbuild,
linux-security-module, kernel-hardening
In-Reply-To: <20190123110349.35882-1-keescook@chromium.org>
Variables declared in a switch statement before any case statements
cannot be initialized, so move all instances out of the switches.
After this, future always-initialized stack variables will work
and not throw warnings like this:
fs/fcntl.c: In function ‘send_sigio_to_task’:
fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
siginfo_t si;
^~
Signed-off-by: Kees Cook <keescook@chromium.org>
---
arch/x86/xen/enlighten_pv.c | 7 ++++---
drivers/char/pcmcia/cm4000_cs.c | 2 +-
drivers/char/ppdev.c | 20 ++++++++-----------
drivers/gpu/drm/drm_edid.c | 4 ++--
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_pm.c | 4 ++--
drivers/net/ethernet/intel/e1000/e1000_main.c | 3 ++-
drivers/tty/n_tty.c | 3 +--
drivers/usb/gadget/udc/net2280.c | 5 ++---
fs/fcntl.c | 3 ++-
mm/shmem.c | 5 +++--
net/core/skbuff.c | 4 ++--
net/ipv6/ip6_gre.c | 4 ++--
net/ipv6/ip6_tunnel.c | 4 ++--
net/openvswitch/flow_netlink.c | 7 +++----
security/tomoyo/common.c | 3 ++-
security/tomoyo/condition.c | 7 ++++---
security/tomoyo/util.c | 4 ++--
18 files changed, 45 insertions(+), 46 deletions(-)
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index c54a493e139a..a79d4b548a08 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -907,14 +907,15 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err)
static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high)
{
int ret;
+#ifdef CONFIG_X86_64
+ unsigned which;
+ u64 base;
+#endif
ret = 0;
switch (msr) {
#ifdef CONFIG_X86_64
- unsigned which;
- u64 base;
-
case MSR_FS_BASE: which = SEGBASE_FS; goto set;
case MSR_KERNEL_GS_BASE: which = SEGBASE_GS_USER; goto set;
case MSR_GS_BASE: which = SEGBASE_GS_KERNEL; goto set;
diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
index 7a4eb86aedac..7211dc0e6f4f 100644
--- a/drivers/char/pcmcia/cm4000_cs.c
+++ b/drivers/char/pcmcia/cm4000_cs.c
@@ -663,6 +663,7 @@ static void monitor_card(struct timer_list *t)
{
struct cm4000_dev *dev = from_timer(dev, t, timer);
unsigned int iobase = dev->p_dev->resource[0]->start;
+ unsigned char flags0;
unsigned short s;
struct ptsreq ptsreq;
int i, atrc;
@@ -731,7 +732,6 @@ static void monitor_card(struct timer_list *t)
}
switch (dev->mstate) {
- unsigned char flags0;
case M_CARDOFF:
DEBUGP(4, dev, "M_CARDOFF\n");
flags0 = inb(REG_FLAGS0(iobase));
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index 1ae77b41050a..d77c97e4f996 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -359,14 +359,19 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
struct pp_struct *pp = file->private_data;
struct parport *port;
void __user *argp = (void __user *)arg;
+ struct ieee1284_info *info;
+ unsigned char reg;
+ unsigned char mask;
+ int mode;
+ s32 time32[2];
+ s64 time64[2];
+ struct timespec64 ts;
+ int ret;
/* First handle the cases that don't take arguments. */
switch (cmd) {
case PPCLAIM:
{
- struct ieee1284_info *info;
- int ret;
-
if (pp->flags & PP_CLAIMED) {
dev_dbg(&pp->pdev->dev, "you've already got it!\n");
return -EINVAL;
@@ -517,15 +522,6 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
port = pp->pdev->port;
switch (cmd) {
- struct ieee1284_info *info;
- unsigned char reg;
- unsigned char mask;
- int mode;
- s32 time32[2];
- s64 time64[2];
- struct timespec64 ts;
- int ret;
-
case PPRSTATUS:
reg = parport_read_status(port);
if (copy_to_user(argp, ®, sizeof(reg)))
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index b506e3622b08..8f93956c1628 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3942,12 +3942,12 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
}
for_each_cea_db(cea, i, start, end) {
+ int sad_count;
+
db = &cea[i];
dbl = cea_db_payload_len(db);
switch (cea_db_tag(db)) {
- int sad_count;
-
case AUDIO_BLOCK:
/* Audio Data Block, contains SADs */
sad_count = min(dbl / 3, 15 - total_sad_count);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3da9c0f9e948..aa1c2ebea456 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11341,6 +11341,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
drm_for_each_connector_iter(connector, &conn_iter) {
struct drm_connector_state *connector_state;
struct intel_encoder *encoder;
+ unsigned int port_mask;
connector_state = drm_atomic_get_new_connector_state(state, connector);
if (!connector_state)
@@ -11354,7 +11355,6 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
WARN_ON(!connector_state->crtc);
switch (encoder->type) {
- unsigned int port_mask;
case INTEL_OUTPUT_DDI:
if (WARN_ON(!HAS_DDI(to_i915(dev))))
break;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a26b4eddda25..c135fdec96b3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -478,9 +478,9 @@ static void vlv_get_fifo_size(struct intel_crtc_state *crtc_state)
struct vlv_fifo_state *fifo_state = &crtc_state->wm.vlv.fifo_state;
enum pipe pipe = crtc->pipe;
int sprite0_start, sprite1_start;
+ uint32_t dsparb, dsparb2, dsparb3;
switch (pipe) {
- uint32_t dsparb, dsparb2, dsparb3;
case PIPE_A:
dsparb = I915_READ(DSPARB);
dsparb2 = I915_READ(DSPARB2);
@@ -1944,6 +1944,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
const struct vlv_fifo_state *fifo_state =
&crtc_state->wm.vlv.fifo_state;
int sprite0_start, sprite1_start, fifo_size;
+ uint32_t dsparb, dsparb2, dsparb3;
if (!crtc_state->fifo_changed)
return;
@@ -1969,7 +1970,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
spin_lock(&dev_priv->uncore.lock);
switch (crtc->pipe) {
- uint32_t dsparb, dsparb2, dsparb3;
case PIPE_A:
dsparb = I915_READ_FW(DSPARB);
dsparb2 = I915_READ_FW(DSPARB2);
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 8fe9af0e2ab7..041062736845 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -3140,8 +3140,9 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
if (skb->data_len && hdr_len == len) {
+ unsigned int pull_size;
+
switch (hw->mac_type) {
- unsigned int pull_size;
case e1000_82544:
/* Make sure we have room to chop off 4 bytes,
* and that the end alignment will work out to
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 5dc9686697cf..eafb39157281 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -634,6 +634,7 @@ static size_t __process_echoes(struct tty_struct *tty)
while (MASK(ldata->echo_commit) != MASK(tail)) {
c = echo_buf(ldata, tail);
if (c == ECHO_OP_START) {
+ unsigned int num_chars, num_bs;
unsigned char op;
int no_space_left = 0;
@@ -652,8 +653,6 @@ static size_t __process_echoes(struct tty_struct *tty)
op = echo_buf(ldata, tail + 1);
switch (op) {
- unsigned int num_chars, num_bs;
-
case ECHO_OP_ERASE_TAB:
if (MASK(ldata->echo_commit) == MASK(tail + 2))
goto not_yet_stored;
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index e7dae5379e04..2b275a574e94 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -2854,16 +2854,15 @@ static void ep_clear_seqnum(struct net2280_ep *ep)
static void handle_stat0_irqs_superspeed(struct net2280 *dev,
struct net2280_ep *ep, struct usb_ctrlrequest r)
{
+ struct net2280_ep *e;
int tmp = 0;
+ u16 status;
#define w_value le16_to_cpu(r.wValue)
#define w_index le16_to_cpu(r.wIndex)
#define w_length le16_to_cpu(r.wLength)
switch (r.bRequest) {
- struct net2280_ep *e;
- u16 status;
-
case USB_REQ_SET_CONFIGURATION:
dev->addressed_state = !w_value;
goto usb3_delegate;
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 083185174c6d..0640b64ecdc2 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -725,6 +725,8 @@ static void send_sigio_to_task(struct task_struct *p,
struct fown_struct *fown,
int fd, int reason, enum pid_type type)
{
+ kernel_siginfo_t si;
+
/*
* F_SETSIG can change ->signum lockless in parallel, make
* sure we read it once and use the same value throughout.
@@ -735,7 +737,6 @@ static void send_sigio_to_task(struct task_struct *p,
return;
switch (signum) {
- kernel_siginfo_t si;
default:
/* Queue a rt signal with the appropriate fd as its
value. We use SI_SIGIO as the source, not
diff --git a/mm/shmem.c b/mm/shmem.c
index 6ece1e2fe76e..0b02624dd8b2 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1721,6 +1721,9 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,
swap_free(swap);
} else {
+ loff_t i_size;
+ pgoff_t off;
+
if (vma && userfaultfd_missing(vma)) {
*fault_type = handle_userfault(vmf, VM_UFFD_MISSING);
return 0;
@@ -1734,8 +1737,6 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,
if (shmem_huge == SHMEM_HUGE_FORCE)
goto alloc_huge;
switch (sbinfo->huge) {
- loff_t i_size;
- pgoff_t off;
case SHMEM_HUGE_NEVER:
goto alloc_nohuge;
case SHMEM_HUGE_WITHIN_SIZE:
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 26d848484912..7597b3fc9d21 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4506,9 +4506,9 @@ static __sum16 *skb_checksum_setup_ip(struct sk_buff *skb,
typeof(IPPROTO_IP) proto,
unsigned int off)
{
- switch (proto) {
- int err;
+ int err;
+ switch (proto) {
case IPPROTO_TCP:
err = skb_maybe_pull_tail(skb, off + sizeof(struct tcphdr),
off + MAX_TCP_HDR_LEN);
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index b1be67ca6768..9aee1add46c0 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -427,9 +427,11 @@ static int ip6gre_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
u8 type, u8 code, int offset, __be32 info)
{
struct net *net = dev_net(skb->dev);
+ struct ipv6_tlv_tnl_enc_lim *tel;
const struct ipv6hdr *ipv6h;
struct tnl_ptk_info tpi;
struct ip6_tnl *t;
+ __u32 teli;
if (gre_parse_header(skb, &tpi, NULL, htons(ETH_P_IPV6),
offset) < 0)
@@ -442,8 +444,6 @@ static int ip6gre_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
return -ENOENT;
switch (type) {
- struct ipv6_tlv_tnl_enc_lim *tel;
- __u32 teli;
case ICMPV6_DEST_UNREACH:
net_dbg_ratelimited("%s: Path to destination invalid or inactive!\n",
t->parms.name);
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 0c6403cf8b52..94ccc7a9037b 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -478,10 +478,12 @@ ip6_tnl_err(struct sk_buff *skb, __u8 ipproto, struct inet6_skb_parm *opt,
struct net *net = dev_net(skb->dev);
u8 rel_type = ICMPV6_DEST_UNREACH;
u8 rel_code = ICMPV6_ADDR_UNREACH;
+ struct ipv6_tlv_tnl_enc_lim *tel;
__u32 rel_info = 0;
struct ip6_tnl *t;
int err = -ENOENT;
int rel_msg = 0;
+ __u32 mtu, teli;
u8 tproto;
__u16 len;
@@ -501,8 +503,6 @@ ip6_tnl_err(struct sk_buff *skb, __u8 ipproto, struct inet6_skb_parm *opt,
err = 0;
switch (*type) {
- struct ipv6_tlv_tnl_enc_lim *tel;
- __u32 mtu, teli;
case ICMPV6_DEST_UNREACH:
net_dbg_ratelimited("%s: Path to destination invalid or inactive!\n",
t->parms.name);
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 691da853bef5..dee2f9516ae8 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -2652,8 +2652,11 @@ static int validate_set(const struct nlattr *a,
u8 mac_proto, __be16 eth_type, bool masked, bool log)
{
const struct nlattr *ovs_key = nla_data(a);
+ const struct ovs_key_ipv4 *ipv4_key;
+ const struct ovs_key_ipv6 *ipv6_key;
int key_type = nla_type(ovs_key);
size_t key_len;
+ int err;
/* There can be only one key in a action */
if (nla_total_size(nla_len(ovs_key)) != nla_len(a))
@@ -2671,10 +2674,6 @@ static int validate_set(const struct nlattr *a,
return -EINVAL;
switch (key_type) {
- const struct ovs_key_ipv4 *ipv4_key;
- const struct ovs_key_ipv6 *ipv6_key;
- int err;
-
case OVS_KEY_ATTR_PRIORITY:
case OVS_KEY_ATTR_SKB_MARK:
case OVS_KEY_ATTR_CT_MARK:
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index c598aa00d5e3..bedbd0518153 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -1583,8 +1583,9 @@ static void tomoyo_read_domain(struct tomoyo_io_buffer *head)
list_for_each_cookie(head->r.domain, &tomoyo_domain_list) {
struct tomoyo_domain_info *domain =
list_entry(head->r.domain, typeof(*domain), list);
+ u8 i;
+
switch (head->r.step) {
- u8 i;
case 0:
if (domain->is_deleted &&
!head->r.print_this_domain_only)
diff --git a/security/tomoyo/condition.c b/security/tomoyo/condition.c
index 8d0e1b9c9c57..c10d903febe5 100644
--- a/security/tomoyo/condition.c
+++ b/security/tomoyo/condition.c
@@ -787,10 +787,11 @@ bool tomoyo_condition(struct tomoyo_request_info *r,
/* Check string expressions. */
if (right == TOMOYO_NAME_UNION) {
const struct tomoyo_name_union *ptr = names_p++;
+ struct tomoyo_path_info *symlink;
+ struct tomoyo_execve *ee;
+ struct file *file;
+
switch (left) {
- struct tomoyo_path_info *symlink;
- struct tomoyo_execve *ee;
- struct file *file;
case TOMOYO_SYMLINK_TARGET:
symlink = obj ? obj->symlink_target : NULL;
if (!symlink ||
diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
index badffc8271c8..8e2bb36df37b 100644
--- a/security/tomoyo/util.c
+++ b/security/tomoyo/util.c
@@ -668,6 +668,8 @@ static bool tomoyo_file_matches_pattern2(const char *filename,
{
while (filename < filename_end && pattern < pattern_end) {
char c;
+ int i, j;
+
if (*pattern != '\\') {
if (*filename++ != *pattern++)
return false;
@@ -676,8 +678,6 @@ static bool tomoyo_file_matches_pattern2(const char *filename,
c = *filename;
pattern++;
switch (*pattern) {
- int i;
- int j;
case '?':
if (c == '/') {
return false;
--
2.17.1
^ permalink raw reply related
* [PATCH 0/3] gcc-plugins: Introduce stackinit plugin
From: Kees Cook @ 2019-01-23 11:03 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Ard Biesheuvel, Laura Abbott, Alexander Popov,
xen-devel, dri-devel, intel-gfx, intel-wired-lan, netdev,
linux-usb, linux-fsdevel, linux-mm, dev, linux-kbuild,
linux-security-module, kernel-hardening
This adds a new plugin "stackinit" that attempts to perform unconditional
initialization of all stack variables[1]. It has wider effects than
GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y since BYREF_ALL does not consider
non-structures. A notable weakness is that padding bytes in many cases
remain uninitialized since GCC treats these bytes as "undefined". I'm
hoping we can improve the compiler (or the plugin) to cover that too.
(It's worth noting that BYREF_ALL actually does handle the padding --
I think this is due to the different method of detecting if initialization
is needed.)
Included is a tree-wide change to move switch variables up and out of
their switch and into the top-level variable declarations.
Included is a set of test cases for evaluating stack initialization,
which checks for padding, different types, etc.
Feedback welcome! :)
-Kees
[1] https://lkml.kernel.org/r/CA+55aFykZL+cSBJjBBts7ebEFfyGPdMzTmLSxKnT_29=j942dA@mail.gmail.com
Kees Cook (3):
treewide: Lift switch variables out of switches
gcc-plugins: Introduce stackinit plugin
lib: Introduce test_stackinit module
arch/x86/xen/enlighten_pv.c | 7 +-
drivers/char/pcmcia/cm4000_cs.c | 2 +-
drivers/char/ppdev.c | 20 +-
drivers/gpu/drm/drm_edid.c | 4 +-
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_pm.c | 4 +-
drivers/net/ethernet/intel/e1000/e1000_main.c | 3 +-
drivers/tty/n_tty.c | 3 +-
drivers/usb/gadget/udc/net2280.c | 5 +-
fs/fcntl.c | 3 +-
lib/Kconfig.debug | 9 +
lib/Makefile | 1 +
lib/test_stackinit.c | 327 ++++++++++++++++++
mm/shmem.c | 5 +-
net/core/skbuff.c | 4 +-
net/ipv6/ip6_gre.c | 4 +-
net/ipv6/ip6_tunnel.c | 4 +-
net/openvswitch/flow_netlink.c | 7 +-
scripts/Makefile.gcc-plugins | 6 +
scripts/gcc-plugins/Kconfig | 9 +
scripts/gcc-plugins/gcc-common.h | 11 +-
scripts/gcc-plugins/stackinit_plugin.c | 79 +++++
security/tomoyo/common.c | 3 +-
security/tomoyo/condition.c | 7 +-
security/tomoyo/util.c | 4 +-
25 files changed, 484 insertions(+), 49 deletions(-)
create mode 100644 lib/test_stackinit.c
create mode 100644 scripts/gcc-plugins/stackinit_plugin.c
--
2.17.1
^ permalink raw reply
* [PATCH 3/3] lib: Introduce test_stackinit module
From: Kees Cook @ 2019-01-23 11:03 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Ard Biesheuvel, Laura Abbott, Alexander Popov,
xen-devel, dri-devel, intel-gfx, intel-wired-lan, netdev,
linux-usb, linux-fsdevel, linux-mm, dev, linux-kbuild,
linux-security-module, kernel-hardening
In-Reply-To: <20190123110349.35882-1-keescook@chromium.org>
Adds test for stack initialization coverage. We have several build options
that control the level of stack variable initialization. This test lets us
visualize which options cover which cases, and provide tests for options
that are currently not available (padding initialization).
All options pass the explicit initialization cases and the partial
initializers (even with padding):
test_stackinit: u8_zero ok
test_stackinit: u16_zero ok
test_stackinit: u32_zero ok
test_stackinit: u64_zero ok
test_stackinit: char_array_zero ok
test_stackinit: small_hole_zero ok
test_stackinit: big_hole_zero ok
test_stackinit: packed_zero ok
test_stackinit: small_hole_dynamic_partial ok
test_stackinit: big_hole_dynamic_partial ok
test_stackinit: packed_static_partial ok
test_stackinit: small_hole_static_partial ok
test_stackinit: big_hole_static_partial ok
The results of the other tests (which contain no explicit initialization),
change based on the build's configured compiler instrumentation.
No options:
test_stackinit: small_hole_static_all FAIL (uninit bytes: 3)
test_stackinit: big_hole_static_all FAIL (uninit bytes: 61)
test_stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3)
test_stackinit: big_hole_dynamic_all FAIL (uninit bytes: 61)
test_stackinit: small_hole_runtime_partial FAIL (uninit bytes: 23)
test_stackinit: big_hole_runtime_partial FAIL (uninit bytes: 127)
test_stackinit: small_hole_runtime_all FAIL (uninit bytes: 3)
test_stackinit: big_hole_runtime_all FAIL (uninit bytes: 61)
test_stackinit: u8 FAIL (uninit bytes: 1)
test_stackinit: u16 FAIL (uninit bytes: 2)
test_stackinit: u32 FAIL (uninit bytes: 4)
test_stackinit: u64 FAIL (uninit bytes: 8)
test_stackinit: char_array FAIL (uninit bytes: 16)
test_stackinit: small_hole FAIL (uninit bytes: 24)
test_stackinit: big_hole FAIL (uninit bytes: 128)
test_stackinit: user FAIL (uninit bytes: 32)
test_stackinit: failures: 16
CONFIG_GCC_PLUGIN_STRUCTLEAK=y
This only tries to initialize structs with __user markings:
test_stackinit: small_hole_static_all FAIL (uninit bytes: 3)
test_stackinit: big_hole_static_all FAIL (uninit bytes: 61)
test_stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3)
test_stackinit: big_hole_dynamic_all FAIL (uninit bytes: 61)
test_stackinit: small_hole_runtime_partial FAIL (uninit bytes: 23)
test_stackinit: big_hole_runtime_partial FAIL (uninit bytes: 127)
test_stackinit: small_hole_runtime_all FAIL (uninit bytes: 3)
test_stackinit: big_hole_runtime_all FAIL (uninit bytes: 61)
test_stackinit: u8 FAIL (uninit bytes: 1)
test_stackinit: u16 FAIL (uninit bytes: 2)
test_stackinit: u32 FAIL (uninit bytes: 4)
test_stackinit: u64 FAIL (uninit bytes: 8)
test_stackinit: char_array FAIL (uninit bytes: 16)
test_stackinit: small_hole FAIL (uninit bytes: 24)
test_stackinit: big_hole FAIL (uninit bytes: 128)
test_stackinit: user ok
test_stackinit: failures: 15
CONFIG_GCC_PLUGIN_STRUCTLEAK=y
CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
This initializes all structures passed by reference (scalars and strings
remain uninitialized, but padding is wiped):
test_stackinit: small_hole_static_all ok
test_stackinit: big_hole_static_all ok
test_stackinit: small_hole_dynamic_all ok
test_stackinit: big_hole_dynamic_all ok
test_stackinit: small_hole_runtime_partial ok
test_stackinit: big_hole_runtime_partial ok
test_stackinit: small_hole_runtime_all ok
test_stackinit: big_hole_runtime_all ok
test_stackinit: u8 FAIL (uninit bytes: 1)
test_stackinit: u16 FAIL (uninit bytes: 2)
test_stackinit: u32 FAIL (uninit bytes: 4)
test_stackinit: u64 FAIL (uninit bytes: 8)
test_stackinit: char_array FAIL (uninit bytes: 16)
test_stackinit: small_hole ok
test_stackinit: big_hole ok
test_stackinit: user ok
test_stackinit: failures: 5
CONFIG_GCC_PLUGIN_STACKINIT=y
This initializes all variables, but has no special padding handling:
test_stackinit: small_hole_static_all FAIL (uninit bytes: 3)
test_stackinit: big_hole_static_all FAIL (uninit bytes: 61)
test_stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3)
test_stackinit: big_hole_dynamic_all FAIL (uninit bytes: 61)
test_stackinit: small_hole_runtime_partial ok
test_stackinit: big_hole_runtime_partial ok
test_stackinit: small_hole_runtime_all ok
test_stackinit: big_hole_runtime_all ok
test_stackinit: u8 ok
test_stackinit: u16 ok
test_stackinit: u32 ok
test_stackinit: u64 ok
test_stackinit: char_array ok
test_stackinit: small_hole ok
test_stackinit: big_hole ok
test_stackinit: user ok
test_stackinit: failures: 4
Signed-off-by: Kees Cook <keescook@chromium.org>
---
lib/Kconfig.debug | 9 ++
lib/Makefile | 1 +
lib/test_stackinit.c | 327 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 337 insertions(+)
create mode 100644 lib/test_stackinit.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index d4df5b24d75e..09788afcccc9 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2001,6 +2001,15 @@ config TEST_OBJAGG
If unsure, say N.
+config TEST_STACKINIT
+ tristate "Test level of stack variable initialization"
+ help
+ Test if the kernel is zero-initializing stack variables
+ from CONFIG_GCC_PLUGIN_STACKINIT, CONFIG_GCC_PLUGIN_STRUCTLEAK,
+ and/or GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
+
+ If unsure, say N.
+
endif # RUNTIME_TESTING_MENU
config MEMTEST
diff --git a/lib/Makefile b/lib/Makefile
index e1b59da71418..c81a66d4d00d 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_TEST_KMOD) += test_kmod.o
obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o
obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o
+obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o
ifeq ($(CONFIG_DEBUG_KOBJECT),y)
CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/test_stackinit.c b/lib/test_stackinit.c
new file mode 100644
index 000000000000..e2ff56a1002a
--- /dev/null
+++ b/lib/test_stackinit.c
@@ -0,0 +1,327 @@
+// SPDX-Licenses: GPLv2
+/*
+ * Test cases for -finit-local-vars and CONFIG_GCC_PLUGIN_STACKINIT.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/string.h>
+
+/* Exfiltration buffer. */
+#define MAX_VAR_SIZE 128
+static char check_buf[MAX_VAR_SIZE];
+
+/* Character array to trigger stack protector in all functions. */
+#define VAR_BUFFER 32
+
+/* Volatile mask to convince compiler to copy memory with 0xff. */
+static volatile u8 forced_mask = 0xff;
+
+/* Location and size tracking to validate fill and test are colocated. */
+static void *fill_start, *target_start;
+static size_t fill_size, target_size;
+
+static bool range_contains(char *haystack_start, size_t haystack_size,
+ char *needle_start, size_t needle_size)
+{
+ if (needle_start >= haystack_start &&
+ needle_start + needle_size <= haystack_start + haystack_size)
+ return true;
+ return false;
+}
+
+#define DO_NOTHING_TYPE_SCALAR(var_type) var_type
+#define DO_NOTHING_TYPE_STRING(var_type) void
+#define DO_NOTHING_TYPE_STRUCT(var_type) void
+
+#define DO_NOTHING_RETURN_SCALAR(ptr) *(ptr)
+#define DO_NOTHING_RETURN_STRING(ptr) /**/
+#define DO_NOTHING_RETURN_STRUCT(ptr) /**/
+
+#define DO_NOTHING_CALL_SCALAR(var, name) \
+ (var) = do_nothing_ ## name(&(var))
+#define DO_NOTHING_CALL_STRING(var, name) \
+ do_nothing_ ## name(var)
+#define DO_NOTHING_CALL_STRUCT(var, name) \
+ do_nothing_ ## name(&(var))
+
+#define FETCH_ARG_SCALAR(var) &var
+#define FETCH_ARG_STRING(var) var
+#define FETCH_ARG_STRUCT(var) &var
+
+#define FILL_SIZE_SCALAR 1
+#define FILL_SIZE_STRING 16
+#define FILL_SIZE_STRUCT 1
+
+#define INIT_CLONE_SCALAR /**/
+#define INIT_CLONE_STRING [FILL_SIZE_STRING]
+#define INIT_CLONE_STRUCT /**/
+
+#define INIT_SCALAR_NONE /**/
+#define INIT_SCALAR_ZERO = 0
+
+#define INIT_STRING_NONE [FILL_SIZE_STRING] /**/
+#define INIT_STRING_ZERO [FILL_SIZE_STRING] = { }
+
+#define INIT_STRUCT_NONE /**/
+#define INIT_STRUCT_ZERO = { }
+#define INIT_STRUCT_STATIC_PARTIAL = { .two = 0, }
+#define INIT_STRUCT_STATIC_ALL = { .one = arg->one, \
+ .two = arg->two, \
+ .three = arg->three, \
+ .four = arg->four, \
+ }
+#define INIT_STRUCT_DYNAMIC_PARTIAL = { .two = arg->two, }
+#define INIT_STRUCT_DYNAMIC_ALL = { .one = arg->one, \
+ .two = arg->two, \
+ .three = arg->three, \
+ .four = arg->four, \
+ }
+#define INIT_STRUCT_RUNTIME_PARTIAL ; \
+ var.two = 0
+#define INIT_STRUCT_RUNTIME_ALL ; \
+ var.one = 0; \
+ var.two = 0; \
+ var.three = 0; \
+ memset(&var.four, 0, \
+ sizeof(var.four))
+
+/*
+ * @name: unique string name for the test
+ * @var_type: type to be tested for zeroing initialization
+ * @which: is this a SCALAR or a STRUCT type?
+ * @init_level: what kind of initialization is performed
+ */
+#define DEFINE_TEST(name, var_type, which, init_level) \
+static noinline int fill_ ## name(unsigned long sp) \
+{ \
+ char buf[VAR_BUFFER + \
+ sizeof(var_type) * FILL_SIZE_ ## which * 4]; \
+ \
+ fill_start = buf; \
+ fill_size = sizeof(buf); \
+ /* Fill variable with 0xFF. */ \
+ memset(fill_start, (char)((sp && 0xff) | forced_mask), \
+ fill_size); \
+ \
+ return (int)buf[0] | (int)buf[sizeof(buf)-1]; \
+} \
+/* no-op to force compiler into ignoring "uninitialized" vars */\
+static noinline DO_NOTHING_TYPE_ ## which(var_type) \
+do_nothing_ ## name(var_type *ptr) \
+{ \
+ /* Will always be true, but compiler doesn't know. */ \
+ if ((unsigned long)ptr > 0x2) \
+ return DO_NOTHING_RETURN_ ## which(ptr); \
+ else \
+ return DO_NOTHING_RETURN_ ## which(ptr + 1); \
+} \
+static noinline int fetch_ ## name(unsigned long sp, \
+ var_type *arg) \
+{ \
+ char buf[VAR_BUFFER]; \
+ var_type var INIT_ ## which ## _ ## init_level; \
+ \
+ target_start = &var; \
+ target_size = sizeof(var); \
+ /* \
+ * Keep this buffer around to make sure we've got a \
+ * stack frame of SOME kind... \
+ */ \
+ memset(buf, (char)(sp && 0xff), sizeof(buf)); \
+ \
+ /* Silence "never initialized" warnings. */ \
+ DO_NOTHING_CALL_ ## which(var, name); \
+ \
+ /* Exfiltrate "var" or field of "var". */ \
+ memcpy(check_buf, target_start, target_size); \
+ \
+ return (int)buf[0] | (int)buf[sizeof(buf) - 1]; \
+} \
+/* Returns 0 on success, 1 on failure. */ \
+static noinline int test_ ## name (void) \
+{ \
+ var_type zero INIT_CLONE_ ## which; \
+ int ignored; \
+ u8 sum = 0, i; \
+ \
+ /* Notice when a new test is larger than expected. */ \
+ BUILD_BUG_ON(sizeof(zero) > MAX_VAR_SIZE); \
+ /* Clear entire check buffer for later bit tests. */ \
+ memset(check_buf, 0x00, sizeof(check_buf)); \
+ \
+ /* Fill clone type with zero for per-field init. */ \
+ memset(&zero, 0x00, sizeof(zero)); \
+ /* Fill stack with 0xFF. */ \
+ ignored = fill_ ##name((unsigned long)&ignored); \
+ /* Extract stack-defined variable contents. */ \
+ ignored = fetch_ ##name((unsigned long)&ignored, \
+ FETCH_ARG_ ## which(zero)); \
+ \
+ /* Validate that compiler lined up fill and target. */ \
+ if (!range_contains(fill_start, fill_size, \
+ target_start, target_size)) { \
+ pr_err(#name ": stack fill missed target!?\n"); \
+ pr_err(#name ": fill %zu wide\n", fill_size); \
+ pr_err(#name ": target offset by %ld\n", \
+ (ssize_t)(uintptr_t)fill_start - \
+ (ssize_t)(uintptr_t)target_start); \
+ return 1; \
+ } \
+ \
+ /* Look for any set bits in the check region. */ \
+ for (i = 0; i < sizeof(check_buf); i++) \
+ sum += (check_buf[i] != 0); \
+ \
+ if (sum == 0) \
+ pr_info(#name " ok\n"); \
+ else \
+ pr_warn(#name " FAIL (uninit bytes: %d)\n", \
+ sum); \
+ \
+ return (sum != 0); \
+}
+
+/* Structure with no padding. */
+struct test_packed {
+ unsigned long one;
+ unsigned long two;
+ unsigned long three;
+ unsigned long four;
+};
+
+/* Simple structure with padding likely to be covered by compiler. */
+struct test_small_hole {
+ size_t one;
+ char two;
+ /* 3 byte padding hole here. */
+ int three;
+ unsigned long four;
+};
+
+/* Try to trigger unhandled padding in a structure. */
+struct test_aligned {
+ u32 internal1;
+ u64 internal2;
+} __aligned(64);
+
+struct test_big_hole {
+ u8 one;
+ u8 two;
+ u8 three;
+ /* 61 byte padding hole here. */
+ struct test_aligned four;
+} __aligned(64);
+
+/* Test if STRUCTLEAK is clearing structs with __user fields. */
+struct test_user {
+ u8 one;
+ char __user *two;
+ unsigned long three;
+ unsigned long four;
+};
+
+/* These should be fully initialized all the time! */
+DEFINE_TEST(u8_zero, u8, SCALAR, ZERO);
+DEFINE_TEST(u16_zero, u16, SCALAR, ZERO);
+DEFINE_TEST(u32_zero, u32, SCALAR, ZERO);
+DEFINE_TEST(u64_zero, u64, SCALAR, ZERO);
+DEFINE_TEST(char_array_zero, unsigned char, STRING, ZERO);
+
+DEFINE_TEST(packed_zero, struct test_packed, STRUCT, ZERO);
+DEFINE_TEST(small_hole_zero, struct test_small_hole, STRUCT, ZERO);
+DEFINE_TEST(big_hole_zero, struct test_big_hole, STRUCT, ZERO);
+
+/* Static initialization: padding may be left uninitialized. */
+DEFINE_TEST(packed_static_partial, struct test_packed, STRUCT, STATIC_PARTIAL);
+DEFINE_TEST(small_hole_static_partial, struct test_small_hole, STRUCT, STATIC_PARTIAL);
+DEFINE_TEST(big_hole_static_partial, struct test_big_hole, STRUCT, STATIC_PARTIAL);
+
+DEFINE_TEST(small_hole_static_all, struct test_small_hole, STRUCT, STATIC_ALL);
+DEFINE_TEST(big_hole_static_all, struct test_big_hole, STRUCT, STATIC_ALL);
+
+/* Dynamic initialization: padding may be left uninitialized. */
+DEFINE_TEST(small_hole_dynamic_partial, struct test_small_hole, STRUCT, DYNAMIC_PARTIAL);
+DEFINE_TEST(big_hole_dynamic_partial, struct test_big_hole, STRUCT, DYNAMIC_PARTIAL);
+
+DEFINE_TEST(small_hole_dynamic_all, struct test_small_hole, STRUCT, DYNAMIC_ALL);
+DEFINE_TEST(big_hole_dynamic_all, struct test_big_hole, STRUCT, DYNAMIC_ALL);
+
+/* Runtime initialization: padding may be left uninitialized. */
+DEFINE_TEST(small_hole_runtime_partial, struct test_small_hole, STRUCT, RUNTIME_PARTIAL);
+DEFINE_TEST(big_hole_runtime_partial, struct test_big_hole, STRUCT, RUNTIME_PARTIAL);
+
+DEFINE_TEST(small_hole_runtime_all, struct test_small_hole, STRUCT, RUNTIME_ALL);
+DEFINE_TEST(big_hole_runtime_all, struct test_big_hole, STRUCT, RUNTIME_ALL);
+
+/* No initialization without compiler instrumentation. */
+DEFINE_TEST(u8, u8, SCALAR, NONE);
+DEFINE_TEST(u16, u16, SCALAR, NONE);
+DEFINE_TEST(u32, u32, SCALAR, NONE);
+DEFINE_TEST(u64, u64, SCALAR, NONE);
+DEFINE_TEST(char_array, unsigned char, STRING, NONE);
+DEFINE_TEST(small_hole, struct test_small_hole, STRUCT, NONE);
+DEFINE_TEST(big_hole, struct test_big_hole, STRUCT, NONE);
+DEFINE_TEST(user, struct test_user, STRUCT, NONE);
+
+static int __init test_stackinit_init(void)
+{
+ unsigned int failures = 0;
+
+ /* These are explicitly initialized and should always pass. */
+ failures += test_u8_zero();
+ failures += test_u16_zero();
+ failures += test_u32_zero();
+ failures += test_u64_zero();
+ failures += test_char_array_zero();
+ failures += test_small_hole_zero();
+ failures += test_big_hole_zero();
+ failures += test_packed_zero();
+
+ /* Padding here appears to be accidentally always initialized. */
+ failures += test_small_hole_dynamic_partial();
+ failures += test_big_hole_dynamic_partial();
+ failures += test_packed_static_partial();
+
+ /* Padding initialization depends on compiler behaviors. */
+ failures += test_small_hole_static_partial();
+ failures += test_big_hole_static_partial();
+ failures += test_small_hole_static_all();
+ failures += test_big_hole_static_all();
+ failures += test_small_hole_dynamic_all();
+ failures += test_big_hole_dynamic_all();
+ failures += test_small_hole_runtime_partial();
+ failures += test_big_hole_runtime_partial();
+ failures += test_small_hole_runtime_all();
+ failures += test_big_hole_runtime_all();
+
+ /* STACKINIT should cover everything from here down. */
+ failures += test_u8();
+ failures += test_u16();
+ failures += test_u32();
+ failures += test_u64();
+ failures += test_char_array();
+
+ /* STRUCTLEAK_BYREF_ALL should cover from here down. */
+ failures += test_small_hole();
+ failures += test_big_hole();
+
+ /* STRUCTLEAK should cover this. */
+ failures += test_user();
+
+ if (failures == 0)
+ pr_info("all tests passed!\n");
+ else
+ pr_err("failures: %u\n", failures);
+
+ return failures ? -EINVAL : 0;
+}
+module_init(test_stackinit_init);
+
+static void __exit test_stackinit_exit(void)
+{ }
+module_exit(test_stackinit_exit);
+
+MODULE_LICENSE("GPL");
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next v7 3/8] devlink: Add port param set command
From: Jiri Pirko @ 2019-01-23 11:03 UTC (permalink / raw)
To: Vasundhara Volam
Cc: davem, michael.chan, jiri, jakub.kicinski, mkubecek, netdev
In-Reply-To: <1547795385-12354-4-git-send-email-vasundhara-v.volam@broadcom.com>
Fri, Jan 18, 2019 at 08:09:40AM CET, vasundhara-v.volam@broadcom.com wrote:
>Add port param set command to set the value for a parameter.
>Value can be set to any of the supported configuration modes.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply
* [PATCH v2 net] ravb: expand rx descriptor data to accommodate hw checksum
From: Simon Horman @ 2019-01-23 11:14 UTC (permalink / raw)
To: David Miller, Sergei Shtylyov
Cc: Magnus Damm, netdev, linux-renesas-soc, Simon Horman
EtherAVB may provide a checksum of packet data appended to packet data. In
order to allow this checksum to be received by the host descriptor data
needs to be enlarged by 2 bytes to accommodate the checksum.
In the case of MTU-sized packets without a VLAN tag the
checksum were already accommodated by virtue of the space reserved for the
VLAN tag. However, a packet of MTU-size with a VLAN tag consumed all
packet data space provided by a descriptor leaving no space for the
trailing checksum.
This was not detected by the driver which incorrectly used the last two
bytes of packet data as the checksum and truncate the packet by two bytes.
This resulted all such packets being dropped.
A work around is to disable RX checksum offload
# ethtool -K eth0 rx off
This patch resolves this problem by increasing the size available for
packet data in RX descriptors by two bytes.
Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board
v2
* Use sizeof(__sum16) directly rather than adding a driver-local
#define for the size of the checksum provided by the hw (2 bytes).
Fixes: 4d86d3818627 ("ravb: RX checksum offload")
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
drivers/net/ethernet/renesas/ravb_main.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ffc1ada4e6da..d28c8f9ca55b 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -343,7 +343,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
int i;
priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) +
- ETH_HLEN + VLAN_HLEN;
+ ETH_HLEN + VLAN_HLEN + sizeof(__sum16);
/* Allocate RX and TX skb rings */
priv->rx_skb[q] = kcalloc(priv->num_rx_ring[q],
@@ -524,13 +524,15 @@ static void ravb_rx_csum(struct sk_buff *skb)
{
u8 *hw_csum;
- /* The hardware checksum is 2 bytes appended to packet data */
- if (unlikely(skb->len < 2))
+ /* The hardware checksum is contained in sizeof(__sum16) (2) bytes
+ * appended to packet data
+ */
+ if (unlikely(skb->len < sizeof(__sum16)))
return;
- hw_csum = skb_tail_pointer(skb) - 2;
+ hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
skb->csum = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
skb->ip_summed = CHECKSUM_COMPLETE;
- skb_trim(skb, skb->len - 2);
+ skb_trim(skb, skb->len - sizeof(__sum16));
}
/* Packet receive function for Ethernet AVB */
--
2.11.0
^ permalink raw reply related
* Re: [PATCH net-next v7 4/8] devlink: Add support for driverinit get value for devlink_port
From: Jiri Pirko @ 2019-01-23 11:08 UTC (permalink / raw)
To: Vasundhara Volam
Cc: davem, michael.chan, jiri, jakub.kicinski, mkubecek, netdev
In-Reply-To: <1547795385-12354-5-git-send-email-vasundhara-v.volam@broadcom.com>
Fri, Jan 18, 2019 at 08:09:41AM CET, vasundhara-v.volam@broadcom.com wrote:
>Add support for "driverinit" configuration mode value for devlink_port
>configuration parameters. Add devlink_port_param_driverinit_value_get()
>function to help the driver get the value from devlink_port.
>
>Also, move the common code to __devlink_param_driverinit_value_get()
>to be used by both device and port params.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> include/net/devlink.h | 8 ++++++
> net/core/devlink.c | 67 ++++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 58 insertions(+), 17 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 98b8a66..09f3f43 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -838,6 +838,14 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> {
> }
>
>+static inline int
>+devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
>+ u32 param_id,
>+ union devlink_param_value *init_val)
>+{
>+ return -EOPNOTSUPP;
>+}
You are missing function declaration in case IS_ENABLED(CONFIG_NET_DEVLINK
Otherwise this looks fine.
>+
> static inline struct devlink_region *
> devlink_region_create(struct devlink *devlink,
> const char *region_name,
[...]
^ permalink raw reply
* Connection Tracking Offload netdev RFC v1.0, part 1/2: command line + implementation
From: Guy Shattah @ 2019-01-23 11:26 UTC (permalink / raw)
To: Marcelo Leitner, Aaron Conole, John Hurley, Simon Horman,
Justin Pettit, Gregory Rose, Eelco Chaudron, Flavio Leitner,
Florian Westphal, Jiri Pirko, Rashid Khan, Sushil Kulkarni,
Andy Gospodarek, Roi Dayan, Yossi Kuperman, Or Gerlitz,
Rony Efraim, davem@davemloft.net
Cc: netdev@vger.kernel.org
--------------------------------------------------------------------------
Connection Tracking Offload netdev RFC v1.0
Part 1/2 - TC with Connection Tracking - command line + implementation
--------------------------------------------------------------------------
OVS recirculation ID is to be translated to TC chain, as described in
https://www.netdevconf.org/2.2/papers/efraim-extendtctoct-talk.pdf
------------------------------------------------------------------------------------
CT Matches:
------------------------------------------------------------------------------------
The ct match acts on ct_state bits or ct variables which were modified as a result from a connection tracking action.
Some of the information can be extacted directly from struct nf_conn and the rest of the information could be taken by using
conntrack_mt...() [/net/netfilter/xt_conntrack.c]
1. ct_state - a new variable
The ct_state match is used to test result of connection tracking.
The bits are set or unset according to the results of the connection tracking module.
The following Match able ct_state items are supported:
* ±trk - Tracked - Been through the connection tracker
* ±new – a new connection
* ±est - Established connection
* ±dnat - Packet’s source address/port was mangled by NAT.
* ±snat - Packet’s destination address/port was mangled by NAT.
* ±inv - Invalid packet
* ±rel – Related to an existing connection
* ±rpl - Reply: Connection must be established
Example #1: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower ct_state
+trk +est -dnat action mirred egress redirect dev eth6"
2. three additional integer variables.
These variables, which can be set from within the ct_action, are introduced:
ct_zone - to commit the connection in (u16) Logically separate connection tracking
table/Multi-tenancy
ct_mark - Attach metadata to particular connections (u32)
ct_label – similar to mark (128 bits)
Example #2: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower
ct_state +trk +est ct_label 10 ct_zone 9 action drop "
Complete list of the flags and their description can be found at:
http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt
------------------------------------------------------------------------------------
CT actions:
------------------------------------------------------------------------------------
The ct_action action sends packet to ConnTrack ( nf_conntrack_in() method) and then updates ct_state bits according to the result from connection tracking.
[1] CT Action has the following possible arguments:
1. commit: Commit the connection to the connection tracking module which will be
stored beyond the lifetime of packet in the pipeline.
2. force: The force flag may be used in addition to commit flag to effectively terminate
the existing connection and start a new one in the current direction.
3. chain = K (chain is similar to ct 'table' in OVS syntax) : Clone packet to send to
connection tracker. When the connection tracker is finished, resume processing
in chain K for that packet. The original packet continues right after the ct(...) action.
4. Set variable: ct_zone, ct_mark, ct_label (see description above)
Example #3:: "tc .... action ct ct_zone 7 commit ct_label 0x0123456789ABCDEF0000111222"
5. NAT: Specifies the address and port translation for the connection being tracked.
Example #4:
"ct_action nat src 10.0.0.1 10.1.1.0" rewrite source ip+port from the list.
Example #5: "tc ... action ct nat src 10.0.0.1 10.1.1.0" rewrite source ip+port
from the list.
Example #6: "tc ... action ct nat auto" rewrite packets automatically from
saved kernel NAT list
-----
[2] CT action also has 3 new parameters
Three new variables which can be set from within the ct_action.
1. ct_zone: 16 bit
2. ct_mark: 32bit
3. ct_label: 128bit
Example #7: tc..... action ct ct_zone 7 commit ct_label x0123456789ABCDEF0000111222
------
[3] NAT action.
Supporting
(1) specific NAT for source
(2) specific NAT for destination
(3) automatic.
TC, when instructed when and how to do so, will do a NAT translation by using the kernel NAT module.
Resulting in a modified skb returning to the following TC chain for further processing
Example #8: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower action
ct commit nat src 10.0.0.0 10.0.0.255"
Commit a new connection to Conntrack and replace NAT the source ip address
Example #9: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower action ct
commit nat auto"
Commit a new connection to Conntrack and replace NAT the source ip address
Additional examples can be found at OVS NAT patch comments:
https://lwn.net/Articles/674868/
[3] match on newly added variables ( ct_zone, ct_mark, ct_label) Example #10: "tc ct_zone 3 ct_mark 0x333 ...."
----------------------------------------
Connection-Tracking action:
----------------------------
TC data path calls Connection Tracking nf_conntrack_in() method with skb which returns connTrack result inside skb->_nfct which is of type struct nf_conn.
Connection-Tracking Match:
----------------------------
connection tracking match can be done using conntrack_mt...() [/net/netfilter/xt_conntrack.c] calls which can be used to match connection tracking information.
Connection-Tracking NAT:
-------------------------------
NAT implementation details are the same as in OVS. As described in:
* https://lwn.net/Articles/674868/
* https://lwn.net/Articles/671459/
* http://www.openvswitch.org/support/ovscon2014/17/1030-conntrack_nat.pdf
Required OVS changes
-------------------------------
1. OVS has to be modified to send Connection-tracking datapath messages to TC
2. OVS datapath has to be enhanced to support enforcement of window-validation
^ permalink raw reply
* Re: [PATCH iproute2-next v2] devlink: Add health command support
From: Aya Levin @ 2019-01-23 11:27 UTC (permalink / raw)
To: David Ahern, netdev@vger.kernel.org, David S. Miller, Jiri Pirko
Cc: Moshe Shemesh, Eran Ben Elisha, Tal Alon, Ariel Almog
In-Reply-To: <4fa4b872-1617-a795-bf86-5c02ed0d2feb@gmail.com>
נכתב על ידי David Ahern, ב־1/23/2019 בשעה 5:37 AM:
> On 1/20/19 2:27 AM, Aya Levin wrote:
>> diff --git a/devlink/devlink.c b/devlink/devlink.c
>> index 3651e90c1159..9fc19668ccd0 100644
>> --- a/devlink/devlink.c
>> +++ b/devlink/devlink.c
>> @@ -1,4 +1,5 @@
>> /*
>> + *
>
> extra newline
>
>> * devlink.c Devlink tool
>> *
>> * This program is free software; you can redistribute it and/or
>> @@ -22,7 +23,7 @@
>> #include <linux/devlink.h>
>> #include <libmnl/libmnl.h>
>> #include <netinet/ether.h>
>> -
>> +#include <sys/sysinfo.h>
>> #include "SNAPSHOT.h"
>> #include "list.h"
>> #include "mnlg.h"
>> @@ -40,6 +41,12 @@
>> #define PARAM_CMODE_DRIVERINIT_STR "driverinit"
>> #define PARAM_CMODE_PERMANENT_STR "permanent"
>>
>> +#define HEALTH_REPORTER_STATE_HEALTHY_STR "healthy"
>> +#define HEALTH_REPORTER_STATE_ERROR_STR "error"
>> +#define HEALTH_REPORTER_GRACE_PERIOD_STR "grace_period"
>> +#define HEALTH_REPORTER_AUTO_RECOVER_STR "auto_recover"
>> +#define HEALTH_REPORTER_TS_LEN 80
>> +
>> static int g_new_line_count;
>>
>> #define pr_err(args...) fprintf(stderr, ##args)
>> @@ -199,6 +206,10 @@ static void ifname_map_free(struct ifname_map *ifname_map)
>> #define DL_OPT_REGION_SNAPSHOT_ID BIT(22)
>> #define DL_OPT_REGION_ADDRESS BIT(23)
>> #define DL_OPT_REGION_LENGTH BIT(24)
>> +#define DL_OPT_HANDLE_HEALTH BIT(25)
>> +#define DL_OPT_HEALTH_REPORTER_NAME BIT(26)
>> +#define DL_OPT_HEALTH_REPORTER_DEV BIT(27)
>> +
>>
>
> extra newline
>
>
>> struct dl_opts {
>> uint32_t present; /* flags of present items */
>> @@ -230,6 +241,10 @@ struct dl_opts {
>> uint32_t region_snapshot_id;
>> uint64_t region_address;
>> uint64_t region_length;
>> + const char *reporter_name;
>> + const char *reporter_param_name;
>> + const char *reporter_param_value;
>> +
>> };
>>
>> struct dl {
>> @@ -959,7 +974,7 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
>> if (err)
>> return err;
>> o_found |= handle_bit;
>> - } else if (o_required & DL_OPT_HANDLE) {
>> + } else if (DL_OPT_HANDLE) {
>
> typo? Seems like o_required is required.
typo should have been o_all
>
>
>> err = dl_argv_handle(dl, &opts->bus_name, &opts->dev_name);
>> if (err)
>> return err;
>> @@ -1178,6 +1193,15 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
>> if (err)
>> return err;
>> o_found |= DL_OPT_REGION_LENGTH;
>> + } else if (dl_argv_match(dl, "reporter") &&
>> + (o_all & DL_OPT_HEALTH_REPORTER_NAME)) {
>> + dl_arg_inc(dl);
>> + err = dl_argv_str(dl, &opts->reporter_name);
>> + if (err)
>> + return err;
>> + o_found |= DL_OPT_HEALTH_REPORTER_NAME;
>> + o_found |= DL_OPT_HANDLE;
>> + break;
>
> why the break? It is the only option that breaks after a match. If it is
> required, please add a comment why for future readers.
>
>
>> } else {
>> pr_err("Unknown option \"%s\"\n", dl_argv(dl));
>> return -EINVAL;
>> @@ -1298,6 +1322,12 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
>> return -EINVAL;
>> }
>>
>> + if ((o_required & DL_OPT_HEALTH_REPORTER_NAME) &&
>> + !(o_found & DL_OPT_HEALTH_REPORTER_NAME)) {
>> + pr_err("Reporter name expected.\n");
>> + return -EINVAL;
>> + }
>
> I realize your are following suit with this change, but these checks for
> 'required and not found' are getting really long. There is a better way
> to do this.
Do you mean somthing like:
#define requiered_not_found(o_found, o_required, flag, msg) \
({ \
if ((o_required & flag) && !(o_found & flag)) { \
pr_err("%s \n", msg); \
return -EINVAL; \
} \
})
>
>
>> +
>> return 0;
>> }
>>
>> @@ -1382,6 +1412,9 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
>> if (opts->present & DL_OPT_REGION_LENGTH)
>> mnl_attr_put_u64(nlh, DEVLINK_ATTR_REGION_CHUNK_LEN,
>> opts->region_length);
>> + if (opts->present & DL_OPT_HEALTH_REPORTER_NAME)
>> + mnl_attr_put_strz(nlh, DEVLINK_ATTR_HEALTH_REPORTER_NAME,
>> + opts->reporter_name);
>> }
>>
>> static int dl_argv_parse_put(struct nlmsghdr *nlh, struct dl *dl,
>> @@ -1513,6 +1546,8 @@ static void __pr_out_handle_start(struct dl *dl, struct nlattr **tb,
>> __pr_out_newline();
>> __pr_out_indent_inc();
>> arr_last_handle_set(dl, bus_name, dev_name);
>> + } else {
>> + __pr_out_indent_inc();
>> }
>> } else {
>> pr_out("%s%s", buf, content ? ":" : "");
>> @@ -5557,11 +5592,502 @@ static int cmd_region(struct dl *dl)
>> return -ENOENT;
>> }
>>
>> +static int cmd_health_set_params(struct dl *dl)
>> +{
>> + struct nlmsghdr *nlh;
>> + uint64_t period;
>> + bool auto_recover;
>> + int err;
>> +
>> + nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_HEALTH_REPORTER_SET,
>> + NLM_F_REQUEST | NLM_F_ACK);
>> + err = dl_argv_parse(dl, DL_OPT_HANDLE |
>> + DL_OPT_HEALTH_REPORTER_NAME, 0);
>> + if (err)
>> + return err;
>> +
>> + err = dl_argv_str(dl, &dl->opts.reporter_param_name);
>> + if (err)
>> + return err;
>> + err = dl_argv_str(dl, &dl->opts.reporter_param_value);
>> + if (err)
>> + return err;
>> + dl_opts_put(nlh, dl);
>> +
>> + if (!strncmp(dl->opts.reporter_param_name,
>> + HEALTH_REPORTER_GRACE_PERIOD_STR, strlen("garce"))) {
>> + err = strtouint64_t(dl->opts.reporter_param_value, &period);
>> + if (err)
>> + goto err_param_value_parse;
>> + mnl_attr_put_u64(nlh, DEVLINK_ATTR_HEALTH_REPORTER_PERIOD,
>> + period);
>> + } else if (!strncmp(dl->opts.reporter_param_name,
>> + HEALTH_REPORTER_AUTO_RECOVER_STR,
>> + strlen("auto"))) {
>> + err = strtobool(dl->opts.reporter_param_value, &auto_recover);
>> + if (err)
>> + goto err_param_value_parse;
>> + mnl_attr_put_u8(nlh, DEVLINK_ATTR_HEALTH_REPORTER_AUTO_REC,
>> + (uint8_t)auto_recover);
>> + } else {
>> + printf("Parameter name: %s is not supported\n",
>> + dl->opts.reporter_param_name);
>> + return -ENOTSUP;
>> + }
>> +
>> + return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
>> +
>> +err_param_value_parse:
>> + pr_err("Value \"%s\" is not a number or not within range\n",
>> + dl->opts.param_value);
>> + return err;
>> +}
>> +
>> +static int cmd_health_dump_clear(struct dl *dl)
>> +{
>> + struct nlmsghdr *nlh;
>> + int err;
>> +
>> + nlh = mnlg_msg_prepare(dl->nlg,
>> + DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR,
>> + NLM_F_REQUEST | NLM_F_ACK);
>> +
>> + err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE |
>> + DL_OPT_HEALTH_REPORTER_NAME, 0);
>> + if (err)
>> + return err;
>> +
>> + dl_opts_put(nlh, dl);
>> + return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
>> +}
>> +
>> +static int health_value_show(struct dl *dl, int type, struct nlattr *nl_data)
>> +{
>> + const char *str;
>> + uint8_t *data;
>> + uint32_t len;
>> + uint64_t u64;
>> + uint32_t u32;
>> + uint16_t u16;
>> + uint8_t u8;
>> + int i;
>> +
>> + switch (type) {
>> + case MNL_TYPE_FLAG:
>> + if (dl->json_output)
>> + jsonw_string(dl->jw, nl_data ? "true" : "false");
>> + else
>> + pr_out("%s ", nl_data ? "true" : "false");
>> + break;
>> + case MNL_TYPE_U8:
>> + u8 = mnl_attr_get_u8(nl_data);
>> + if (dl->json_output)
>> + jsonw_uint(dl->jw, u8);
>> + else
>> + pr_out("%u ", u8);
>> + break;
>> + case MNL_TYPE_U16:
>> + u16 = mnl_attr_get_u16(nl_data);
>> + if (dl->json_output)
>> + jsonw_uint(dl->jw, u16);
>> + else
>> + pr_out("%u ", u16);
>> + break;
>> + case MNL_TYPE_U32:
>> + u32 = mnl_attr_get_u32(nl_data);
>> + if (dl->json_output)
>> + jsonw_uint(dl->jw, u32);
>> + else
>> + pr_out("%u ", u32);
>> + break;
>> + case MNL_TYPE_U64:
>> + u64 = mnl_attr_get_u64(nl_data);
>> + if (dl->json_output)
>> + jsonw_u64(dl->jw, u64);
>> + else
>> + pr_out("%lu ", u64);
>> + break;
>> + case MNL_TYPE_STRING:
>> + case MNL_TYPE_NUL_STRING:
>> + str = mnl_attr_get_str(nl_data);
>> + if (dl->json_output)
>> + jsonw_string(dl->jw, str);
>> + else
>> + pr_out("%s ", str);
>> + break;
>> + case MNL_TYPE_BINARY:
>> + len = mnl_attr_get_payload_len(nl_data);
>> + data = mnl_attr_get_payload(nl_data);
>> + i = 0;
>> + while (i < len) {
>> + if (dl->json_output)
>> + jsonw_printf(dl->jw, "%d", data[i]);
>> + else
>> + pr_out("%02x ", data[i]);
>> + i++;
>> + }
>
> If 'len' is an arbitrary size then line lengths can be ridiculous here.
>
>
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + return MNL_CB_OK;
>> +}
>> +
>
> ...
>
>> +static int jiffies_to_logtime(uint64_t jiffies, char *output)
>> +{
>> + struct sysinfo s_info;
>> + struct tm *info;
>> + time_t now, sec;
>> + int err;
>> +
>> + time(&now);
>> + info = localtime(&now);
>> + strftime(output, HEALTH_REPORTER_TS_LEN, "%Y-%b-%d %H:%M:%S", info);
>
> This strftime should really be done in the error path of sysinfo as
> opposed to every call to this function calling strftime twice.
>
>> + err = sysinfo(&s_info);
>> + if (err)
>> + return err;
>> + /*substruct uptime in sec from now
>> + * add jiffies and 5 minutes factor*/
>
> fix the comment style.
>
>
>> + sec = now - (s_info.uptime - jiffies/1000) + 300;
>> + info = localtime(&sec);
>> + strftime(output, HEALTH_REPORTER_TS_LEN, "%Y-%b-%d %H:%M:%S", info);
>> +
>> + return 0;
>> +}
>> +
>
^ permalink raw reply
* Connection Tracking Offload netdev RFC v1.0, Part 2/2 - TC with Connection Tracking - hardware offloading and Netfilter changes
From: Guy Shattah @ 2019-01-23 11:28 UTC (permalink / raw)
To: Marcelo Leitner, Aaron Conole, John Hurley, Simon Horman,
Justin Pettit, Gregory Rose, Eelco Chaudron, Flavio Leitner,
Florian Westphal, Jiri Pirko, Rashid Khan, Sushil Kulkarni,
Andy Gospodarek, Roi Dayan, Yossi Kuperman, Or Gerlitz,
Rony Efraim, davem@davemloft.net, Pablo Neira Ayuso
Cc: netdev@vger.kernel.org
-------------------------------------------------------------------------
Connection Tracking Offload netdev RFC v1.0
Part 2/2 - TC with Connection Tracking - hardware offloading and Netfilter changes
--------------------------------------------------------------------------
This part continues the discussion of hardware offload.
All details regarding hardware specific implementation and driver were deliberately
omitted from this document. as this document should be one fit all vendors
TC command-line interface
--------------------------------
In case of connection tracking hardware-offload TC should reject all skip_software rules. This is done to make
sure sw and hw are always in sync.
It has to be forced by tc in order to prevent cases where there is a chain with
connection tracking (a rule which is offloaded) and the following
chain (which does not contain hardware offload action or match) was not offloaded.
A suggestion:
* On the first time a ct_action or ct_state is used tc would scan
all existing chains and refuse to insert rule if at least one skip_software rule is found.
If everything is OK then raise a flag to mark that all rules from now on must
be both in software and hardware.
* Any further rules will check this flag.
* This flag is clear when all rules in TC are cleared.
I'm certain that we could find more solutions if this one is not sufficient.
Offloading ct_state matches:
--------------------------------
The current offload interface used by TC is ndo_setup_tc().
This interface is used to send a tc chain to the hardware.
The interface is kept the same for commands containing ct_state.
The only difference is in the underlying driver/hardware implementation.
1. It is the duty of the driver to convert 'matches' such as:
ct_zone=XX,ct_mark=XX,,ct_label=XX,ct_state=±trk,±new,±est,±dnat,±snat,±inv,±rel,±rpl
properly to the hardware interface.
2. It is the duty of the driver to make sure that in case of ambiguity, where
hardware is not capable of making a decision on a packet path to send
packet to the software for further processing [see section on fallback to software]
3. please note that Hardware may or may not have the following capabilities:
(a) packet check-sum (layer 3/4)
(b) TCP window-validation
(c) flow counters/stats
(d) other?
Offloading ct_action / changes in tc data-path:
--------------------------------------------------
While on pure software tc connection tracking implementation the packet is sent to connection-tracking
to retrieve result into skb->_nfct and futher matches are used on this struct to make routing decisions,
On tc with offload support there is an additional test:
If packet is part of an established connection a decision is made to offload connection to hardware.
To offload this connection a 'flow offload' infrastructure developed by Pablo is used,
( This infrastructure is enhanced to support connection tracking as described below.[see section on netfilter changes] )
The packet is sent to nft_flow_offload_eval() along with additional data for offload evaluation.
Such additional data is:
* struct _nfct
* from which current flags such as ±rpl/±rel can be used by the driver to set ct_state on
established connection.
* TCP window information
* Any other additional information.
* Originating tc chain id, from which the offload took place.
* flow (connection) id
Possible syncronization issues:
---------------------------------
When connection are offloaded there is a short latency until rule is active in hardware.
This means that packets may arrive to TC after connection has been offloaded.
While this doesn't introduce any issue on UDP+ICMP it might be an issue
on TCP due to TCP window issues. In this case I'd suggest to offload the connection with a
large tcp window and allow the hardware (where window-validation is supported) rescale the window back.
Netfilter changes
---------------------
Once a packet passed through a chain which includes a CT action resulted with 'established' state
TC to send the packet to Netfilter for offload evaluation [ nft_flow_offload_eval()] and then
and flow should be offloaded.
Changes:
1. On Pablo's patches, he had added a new ndo for hardware offload:
https://github.com/orgcandman/linux-next-work/commit/351bd1303c2ae5de0c4512f0cdf7b61508df0777#diff-cf45716f2ff8e67a1727be77c3b894feR1392
However, During discussions in netfilter workshop,(May 2018) Pablo and Jiri Pirko have agreed that both NDOs basically
do the same thing - hardware offload. Hence it would be wiser to use ndo_setup_tc call from netfilter to do the
offload, rather than adding a whole new NDO.
Another suggestion was made it to rename ndo_setup_tc in the future, to reflect that it is not only used from TC.
Please read the following paper from netdev, the paper discusses this solution in depth:
https://www.files.netdevconf.org/d/c5b1c9709ca743d79a63/
2. in case of tc NAT, the NAT header-rewrite has to be a part of the
connection-tracking offload. hence TC will supply Netfilter offload
code not only the chain ID from which the offload took place but
also information on how to do NAT translation.
2. Netfilter to maintain two separate lists of offloaded connections.
Each list is offloaded flows is kept inside: struct nf_flowtable *flowtable
(currently - there is only one list)
2.1 The first list is a list of 'soft' offloaded connections.
In this list Netfilter iterates and is responsible for flow aging and flow termination.
For flow aging it iterates over all the flows.
For flow termination it will be using FIN packets arriving from TC.
2.2 The second list is a list of 'hardware' offloaded connections.
This is a software representation of the psychical list representation in hardware for offloaded connections.
Flows in this list are aged and terminated by the hardware/driver by event/callback.
3. On reaching full HW capacity:
In case where hardware capacity is reached Netfilter should mark the flow
(a new addition inside struct nf_conn or flow offload entry)
and put it on a 'pending list'.
Once the 'offloaded flows list' has new empty spots (released by events)
then it should try to offload again.
Another possibility (maybe in phase two?) is have some kind of smarter LRU algorithm
where some of the connections are offloaded, some are not and there is an algorithm to
decide and switch between hardware and software, based on traffic (stats/counters).
4. Expiration mechanism [in software]:
The current Expiration in Connection Tracking:
Lazy expiration: old connections are removed when new connections arrive.
Relatively long time: over 5 Days for established TCP connection About 180 secs for UDP
Expiration in Netfilter flow offload :
Every once in a while scan all existing offloaded flows and query driver to see if flow has expired [suggested queries to driver in a code that was not submitted upstream:
if (nf_flow_has_expired(flow) ||
nf_flow_is_dying(flow)) {
Suggested expiration algorithm [to be implemented in netfilter flow-offload or the driver]:
Instead of scanning millions of items per one second scan a small number of items per seconds.
Covering all of them twice within the timeout period:
With N offloaded connections - Every K seconds sample N/2k of the flows in the systems.
K= 180 secs for udp/icmp
K= 3600 secs for tcp
Final expiration value either configurable or constant
5. counters - NetFilter has to be enhanced to pull counter (per flow) from hardware
and update stats per connection/flow.
6. megaflows - Netfilter has to be enhanced with API to support deletion of
multiple flows according to the OVS recirculation id/tc chain id.
As dicussed - this will be either a blocking call or async.
7. Fallback - Netfilter has to be enhanced with API to support case where
a flow has to be moved from HW back to SW and then:
(1) never be moved to HW again OR (2) attemp to move back to HW where possible.
Handling OVS rule eviction:
------------------------------
The way OVS offload is being done today is by OVS sending a copy of the OVS-Kernel data-path netlink
messages to TC. OVS then samples counters/stats to see if this flow/connection is still valid
and has not aged. OVS may decide to evict this rule due to aging or due to user manually removing
the openflow rule.
When a rule was removed (a rule containing ct_action command) then a netlink command is sent to TC
TC should then send a command to netfilter offload code (similar to the event mentioned before) to
empty the offloaded connection list.
Regarding counter/stats: for a single chain with a ct_action we need to
pull counters/stats from all the offloaded connections. an accumulative counter.
Fallback to software
-------------------------
While hardware is processing a packet there is a possibility that hardware
reaches a state/flow table which does not contain sufficient information on how to
continue processing.
Such cases may happen when originating flow-table sends a packet to non-existing
target flow table. When hardware capabilities are not sufficient to make a decision,
When there is ambiguity or when packet failed all tests and is sent to software
for further analysis and possibly other cases.
We should note that this may happen in any state of the processing:
It may happen after packet has been decapsulated or after encapsulation.
After header-rewrite (such as NAT) or after some meta-data was placed on the packet
or the connection (ct_label data, for example).
We define two acceptable behaviors:
1. Fallback from a middle of processing to a tc chain which is not the entry one (i.e. chain zero)
the mechanism should be able to fully deliver a packet with all meta-data
to upper layer with exact middle point of processing. to continue processing.
2. Fallback from a middle of processing to a start/entry-point of processing.
this is a private case of #1. Where last flow table is zero.
In this case a replica of the original unmodified packet has to be sent to the start/entry-point of upper-level.
Any other behavior is not acceptable, since software cannot handle cases
where the packet header has been modified without knowning where the process stopped.
In addition to the two described behavior, we also allow a fallback into a middle of
a tc chain.
Why is it required?
In case of a tc rule that does decapsulation/header-rewrite and then connection tracking
it is possible that a packet will go trough decapsulation/header-rewrite and then fail
on connection-tracking in hardware (due to connection not being offloaded yet).
in this case the software (i.e. tc) may receive a malformed packet which does
not match accurately at the tc chain. Hence there is a need to enter the packet
into a middle a chain, after matches were done and all the actions, except for connection tracking.
i.e.; the packet is sent to the equivalent tc chain to do connection-tracking there.
Hence, there is support for fallback into (1) a beginning of tc chain and (2) right into the middle of it before CT.
----
Now lets talk about fallbacks:
1. Fallback from hardware/driver to TC
The driver should be able to restore information (ct_zone/mark/label/last flow table)
into the packet.
We suggest that ct_zone/ct_mark/ct_label/ct_state are restored into struct nf_conn (skb->_nfct)
and the last flow_table is restored into skb->cb (we have space there we could use)
On receiving packet with skb->cb set to certain value tc will start processing
the packet from a chain specific in skb->cb .
2. Fallback from TC to OVS.
There is a need to add a new logic into OVS which supports handling of a packet
from a specific recirculation id/chain/flow table.
Currently OVS-kernel clones the skb and if it fails processing (in kernel)
then the original packet is sent to OVS-userspace.
This cannot work for our case since TC doesn't keep the original copy of the
packet (before header-rewrite/decapsulation/etc) and understandably, the packet
cannot be restored to its original form.
We suggest to do the TC->OVS path in the same manner as Driver->TC path.
a packet is reassembled and restored by the driver and injected into TC
(along with meta-data on skb->cv). If TC lacks the information on how to
proceed the packet is left unchanged and the next stop is OVS which has to be
extended with new logic on how to proceed.
^ permalink raw reply
* [RFC] Connection Tracking Offload netdev RFC v1.0, Part 2/2 - TC with Connection Tracking - hardware offloading and Netfilter changes
From: Guy Shattah @ 2019-01-23 11:29 UTC (permalink / raw)
To: Guy Shattah, Marcelo Leitner, Aaron Conole, John Hurley,
Simon Horman, Justin Pettit, Gregory Rose, Eelco Chaudron,
Flavio Leitner, Florian Westphal, Jiri Pirko, Rashid Khan,
Sushil Kulkarni, Andy Gospodarek, Roi Dayan, Yossi Kuperman,
Or Gerlitz, Rony Efraim, davem@davemloft.net, Pablo Neira Ayuso
Cc: netdev@vger.kernel.org
-------------------------------------------------------------------------
Connection Tracking Offload netdev RFC v1.0 Part 2/2 - TC with Connection Tracking - hardware offloading and Netfilter changes
--------------------------------------------------------------------------
This part continues the discussion of hardware offload.
All details regarding hardware specific implementation and driver were deliberately omitted from this document. as this document should be one fit all vendors
TC command-line interface
--------------------------------
In case of connection tracking hardware-offload TC should reject all skip_software rules. This is done to make
sure sw and hw are always in sync.
It has to be forced by tc in order to prevent cases where there is a chain with
connection tracking (a rule which is offloaded) and the following
chain (which does not contain hardware offload action or match) was not offloaded.
A suggestion:
* On the first time a ct_action or ct_state is used tc would scan
all existing chains and refuse to insert rule if at least one skip_software rule is found.
If everything is OK then raise a flag to mark that all rules from now on must
be both in software and hardware.
* Any further rules will check this flag.
* This flag is clear when all rules in TC are cleared.
I'm certain that we could find more solutions if this one is not sufficient.
Offloading ct_state matches:
--------------------------------
The current offload interface used by TC is ndo_setup_tc().
This interface is used to send a tc chain to the hardware.
The interface is kept the same for commands containing ct_state.
The only difference is in the underlying driver/hardware implementation.
1. It is the duty of the driver to convert 'matches' such as:
ct_zone=XX,ct_mark=XX,,ct_label=XX,ct_state=±trk,±new,±est,±dnat,±snat,±inv,±rel,±rpl
properly to the hardware interface.
2. It is the duty of the driver to make sure that in case of ambiguity, where
hardware is not capable of making a decision on a packet path to send
packet to the software for further processing [see section on fallback to software]
3. please note that Hardware may or may not have the following capabilities:
(a) packet check-sum (layer 3/4)
(b) TCP window-validation
(c) flow counters/stats
(d) other?
Offloading ct_action / changes in tc data-path:
--------------------------------------------------
While on pure software tc connection tracking implementation the packet is sent to connection-tracking
to retrieve result into skb->_nfct and futher matches are used on this struct to make routing decisions,
On tc with offload support there is an additional test:
If packet is part of an established connection a decision is made to offload connection to hardware.
To offload this connection a 'flow offload' infrastructure developed by Pablo is used,
( This infrastructure is enhanced to support connection tracking as described below.[see section on netfilter changes] )
The packet is sent to nft_flow_offload_eval() along with additional data for offload evaluation.
Such additional data is:
* struct _nfct
* from which current flags such as ±rpl/±rel can be used by the driver to set ct_state on
established connection.
* TCP window information
* Any other additional information.
* Originating tc chain id, from which the offload took place.
* flow (connection) id
Possible syncronization issues:
---------------------------------
When connection are offloaded there is a short latency until rule is active in hardware.
This means that packets may arrive to TC after connection has been offloaded.
While this doesn't introduce any issue on UDP+ICMP it might be an issue
on TCP due to TCP window issues. In this case I'd suggest to offload the connection with a
large tcp window and allow the hardware (where window-validation is supported) rescale the window back.
Netfilter changes
---------------------
Once a packet passed through a chain which includes a CT action resulted with 'established' state
TC to send the packet to Netfilter for offload evaluation [ nft_flow_offload_eval()] and then
and flow should be offloaded.
Changes:
1. On Pablo's patches, he had added a new ndo for hardware offload:
https://github.com/orgcandman/linux-next-work/commit/351bd1303c2ae5de0c4512f0cdf7b61508df0777#diff-cf45716f2ff8e67a1727be77c3b894feR1392
However, During discussions in netfilter workshop,(May 2018) Pablo and Jiri Pirko have agreed that both NDOs basically
do the same thing - hardware offload. Hence it would be wiser to use ndo_setup_tc call from netfilter to do the
offload, rather than adding a whole new NDO.
Another suggestion was made it to rename ndo_setup_tc in the future, to reflect that it is not only used from TC.
Please read the following paper from netdev, the paper discusses this solution in depth:
https://www.files.netdevconf.org/d/c5b1c9709ca743d79a63/
2. in case of tc NAT, the NAT header-rewrite has to be a part of the
connection-tracking offload. hence TC will supply Netfilter offload
code not only the chain ID from which the offload took place but
also information on how to do NAT translation.
2. Netfilter to maintain two separate lists of offloaded connections.
Each list is offloaded flows is kept inside: struct nf_flowtable *flowtable
(currently - there is only one list)
2.1 The first list is a list of 'soft' offloaded connections.
In this list Netfilter iterates and is responsible for flow aging and flow termination.
For flow aging it iterates over all the flows.
For flow termination it will be using FIN packets arriving from TC.
2.2 The second list is a list of 'hardware' offloaded connections.
This is a software representation of the psychical list representation in hardware for offloaded connections.
Flows in this list are aged and terminated by the hardware/driver by event/callback.
3. On reaching full HW capacity:
In case where hardware capacity is reached Netfilter should mark the flow
(a new addition inside struct nf_conn or flow offload entry)
and put it on a 'pending list'.
Once the 'offloaded flows list' has new empty spots (released by events)
then it should try to offload again.
Another possibility (maybe in phase two?) is have some kind of smarter LRU algorithm
where some of the connections are offloaded, some are not and there is an algorithm to
decide and switch between hardware and software, based on traffic (stats/counters).
4. Expiration mechanism [in software]:
The current Expiration in Connection Tracking:
Lazy expiration: old connections are removed when new connections arrive.
Relatively long time: over 5 Days for established TCP connection About 180 secs for UDP
Expiration in Netfilter flow offload :
Every once in a while scan all existing offloaded flows and query driver to see if flow has expired [suggested queries to driver in a code that was not submitted upstream:
if (nf_flow_has_expired(flow) ||
nf_flow_is_dying(flow)) {
Suggested expiration algorithm [to be implemented in netfilter flow-offload or the driver]:
Instead of scanning millions of items per one second scan a small number of items per seconds.
Covering all of them twice within the timeout period:
With N offloaded connections - Every K seconds sample N/2k of the flows in the systems.
K= 180 secs for udp/icmp
K= 3600 secs for tcp
Final expiration value either configurable or constant
5. counters - NetFilter has to be enhanced to pull counter (per flow) from hardware
and update stats per connection/flow.
6. megaflows - Netfilter has to be enhanced with API to support deletion of
multiple flows according to the OVS recirculation id/tc chain id.
As dicussed - this will be either a blocking call or async.
7. Fallback - Netfilter has to be enhanced with API to support case where
a flow has to be moved from HW back to SW and then:
(1) never be moved to HW again OR (2) attemp to move back to HW where possible.
Handling OVS rule eviction:
------------------------------
The way OVS offload is being done today is by OVS sending a copy of the OVS-Kernel data-path netlink
messages to TC. OVS then samples counters/stats to see if this flow/connection is still valid
and has not aged. OVS may decide to evict this rule due to aging or due to user manually removing
the openflow rule.
When a rule was removed (a rule containing ct_action command) then a netlink command is sent to TC
TC should then send a command to netfilter offload code (similar to the event mentioned before) to
empty the offloaded connection list.
Regarding counter/stats: for a single chain with a ct_action we need to
pull counters/stats from all the offloaded connections. an accumulative counter.
Fallback to software
-------------------------
While hardware is processing a packet there is a possibility that hardware
reaches a state/flow table which does not contain sufficient information on how to
continue processing.
Such cases may happen when originating flow-table sends a packet to non-existing
target flow table. When hardware capabilities are not sufficient to make a decision,
When there is ambiguity or when packet failed all tests and is sent to software
for further analysis and possibly other cases.
We should note that this may happen in any state of the processing:
It may happen after packet has been decapsulated or after encapsulation.
After header-rewrite (such as NAT) or after some meta-data was placed on the packet
or the connection (ct_label data, for example).
We define two acceptable behaviors:
1. Fallback from a middle of processing to a tc chain which is not the entry one (i.e. chain zero)
the mechanism should be able to fully deliver a packet with all meta-data
to upper layer with exact middle point of processing. to continue processing.
2. Fallback from a middle of processing to a start/entry-point of processing.
this is a private case of #1. Where last flow table is zero.
In this case a replica of the original unmodified packet has to be sent to the start/entry-point of upper-level.
Any other behavior is not acceptable, since software cannot handle cases
where the packet header has been modified without knowning where the process stopped.
In addition to the two described behavior, we also allow a fallback into a middle of
a tc chain.
Why is it required?
In case of a tc rule that does decapsulation/header-rewrite and then connection tracking
it is possible that a packet will go trough decapsulation/header-rewrite and then fail
on connection-tracking in hardware (due to connection not being offloaded yet).
in this case the software (i.e. tc) may receive a malformed packet which does
not match accurately at the tc chain. Hence there is a need to enter the packet
into a middle a chain, after matches were done and all the actions, except for connection tracking.
i.e.; the packet is sent to the equivalent tc chain to do connection-tracking there.
Hence, there is support for fallback into (1) a beginning of tc chain and (2) right into the middle of it before CT.
----
Now lets talk about fallbacks:
1. Fallback from hardware/driver to TC
The driver should be able to restore information (ct_zone/mark/label/last flow table)
into the packet.
We suggest that ct_zone/ct_mark/ct_label/ct_state are restored into struct nf_conn (skb->_nfct)
and the last flow_table is restored into skb->cb (we have space there we could use)
On receiving packet with skb->cb set to certain value tc will start processing
the packet from a chain specific in skb->cb .
2. Fallback from TC to OVS.
There is a need to add a new logic into OVS which supports handling of a packet
from a specific recirculation id/chain/flow table.
Currently OVS-kernel clones the skb and if it fails processing (in kernel)
then the original packet is sent to OVS-userspace.
This cannot work for our case since TC doesn't keep the original copy of the
packet (before header-rewrite/decapsulation/etc) and understandably, the packet
cannot be restored to its original form.
We suggest to do the TC->OVS path in the same manner as Driver->TC path.
a packet is reassembled and restored by the driver and injected into TC
(along with meta-data on skb->cv). If TC lacks the information on how to
proceed the packet is left unchanged and the next stop is OVS which has to be
extended with new logic on how to proceed.
^ permalink raw reply
* [RFC] Connection Tracking Offload netdev RFC v1.0, part 1/2: command line + implementation
From: Guy Shattah @ 2019-01-23 11:29 UTC (permalink / raw)
To: Guy Shattah, Marcelo Leitner, Aaron Conole, John Hurley,
Simon Horman, Justin Pettit, Gregory Rose, Eelco Chaudron,
Flavio Leitner, Florian Westphal, Jiri Pirko, Rashid Khan,
Sushil Kulkarni, Andy Gospodarek, Roi Dayan, Yossi Kuperman,
Or Gerlitz, Rony Efraim, davem@davemloft.net
Cc: netdev@vger.kernel.org
--------------------------------------------------------------------------
Connection Tracking Offload netdev RFC v1.0 Part 1/2 - TC with Connection Tracking - command line + implementation
--------------------------------------------------------------------------
OVS recirculation ID is to be translated to TC chain, as described in https://www.netdevconf.org/2.2/papers/efraim-extendtctoct-talk.pdf
------------------------------------------------------------------------------------
CT Matches:
------------------------------------------------------------------------------------
The ct match acts on ct_state bits or ct variables which were modified as a result from a connection tracking action.
Some of the information can be extacted directly from struct nf_conn and the rest of the information could be taken by using
conntrack_mt...() [/net/netfilter/xt_conntrack.c]
1. ct_state - a new variable
The ct_state match is used to test result of connection tracking.
The bits are set or unset according to the results of the connection tracking module.
The following Match able ct_state items are supported:
* ±trk - Tracked - Been through the connection tracker
* ±new – a new connection
* ±est - Established connection
* ±dnat - Packet’s source address/port was mangled by NAT.
* ±snat - Packet’s destination address/port was mangled by NAT.
* ±inv - Invalid packet
* ±rel – Related to an existing connection
* ±rpl - Reply: Connection must be established
Example #1: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower ct_state
+trk +est -dnat action mirred egress redirect dev eth6"
2. three additional integer variables.
These variables, which can be set from within the ct_action, are introduced:
ct_zone - to commit the connection in (u16) Logically separate connection tracking
table/Multi-tenancy
ct_mark - Attach metadata to particular connections (u32)
ct_label – similar to mark (128 bits)
Example #2: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower
ct_state +trk +est ct_label 10 ct_zone 9 action drop "
Complete list of the flags and their description can be found at:
http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt
------------------------------------------------------------------------------------
CT actions:
------------------------------------------------------------------------------------
The ct_action action sends packet to ConnTrack ( nf_conntrack_in() method) and then updates ct_state bits according to the result from connection tracking.
[1] CT Action has the following possible arguments:
1. commit: Commit the connection to the connection tracking module which will be
stored beyond the lifetime of packet in the pipeline.
2. force: The force flag may be used in addition to commit flag to effectively terminate
the existing connection and start a new one in the current direction.
3. chain = K (chain is similar to ct 'table' in OVS syntax) : Clone packet to send to
connection tracker. When the connection tracker is finished, resume processing
in chain K for that packet. The original packet continues right after the ct(...) action.
4. Set variable: ct_zone, ct_mark, ct_label (see description above)
Example #3:: "tc .... action ct ct_zone 7 commit ct_label 0x0123456789ABCDEF0000111222"
5. NAT: Specifies the address and port translation for the connection being tracked.
Example #4:
"ct_action nat src 10.0.0.1 10.1.1.0" rewrite source ip+port from the list.
Example #5: "tc ... action ct nat src 10.0.0.1 10.1.1.0" rewrite source ip+port
from the list.
Example #6: "tc ... action ct nat auto" rewrite packets automatically from
saved kernel NAT list
-----
[2] CT action also has 3 new parameters
Three new variables which can be set from within the ct_action.
1. ct_zone: 16 bit
2. ct_mark: 32bit
3. ct_label: 128bit
Example #7: tc..... action ct ct_zone 7 commit ct_label x0123456789ABCDEF0000111222
------
[3] NAT action.
Supporting
(1) specific NAT for source
(2) specific NAT for destination
(3) automatic.
TC, when instructed when and how to do so, will do a NAT translation by using the kernel NAT module.
Resulting in a modified skb returning to the following TC chain for further processing
Example #8: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower action
ct commit nat src 10.0.0.0 10.0.0.255"
Commit a new connection to Conntrack and replace NAT the source ip address
Example #9: "tc filter add dev eth5 protocol ip parent ffff: chain 100 flower action ct
commit nat auto"
Commit a new connection to Conntrack and replace NAT the source ip address
Additional examples can be found at OVS NAT patch comments:
https://lwn.net/Articles/674868/
[3] match on newly added variables ( ct_zone, ct_mark, ct_label) Example #10: "tc ct_zone 3 ct_mark 0x333 ...."
----------------------------------------
Connection-Tracking action:
----------------------------
TC data path calls Connection Tracking nf_conntrack_in() method with skb which returns connTrack result inside skb->_nfct which is of type struct nf_conn.
Connection-Tracking Match:
----------------------------
connection tracking match can be done using conntrack_mt...() [/net/netfilter/xt_conntrack.c] calls which can be used to match connection tracking information.
Connection-Tracking NAT:
-------------------------------
NAT implementation details are the same as in OVS. As described in:
* https://lwn.net/Articles/674868/
* https://lwn.net/Articles/671459/
* http://www.openvswitch.org/support/ovscon2014/17/1030-conntrack_nat.pdf
Required OVS changes
-------------------------------
1. OVS has to be modified to send Connection-tracking datapath messages to TC 2. OVS datapath has to be enhanced to support enforcement of window-validation
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox