* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Jason Wang @ 2018-08-03 2:38 UTC (permalink / raw)
To: Toshiaki Makita, Tonghao Zhang
Cc: mst, virtualization, Linux Kernel Network Developers
In-Reply-To: <e2917cb0-f8dd-90c7-5a44-5333ad1d1ee8@lab.ntt.co.jp>
On 2018年08月02日 17:57, Toshiaki Makita wrote:
> On 2018/08/02 18:23, Jason Wang wrote:
>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
>>> On 2018/08/02 17:18, Jason Wang wrote:
>>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
>>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>>>>>> + struct vhost_virtqueue *rvq,
>>>>>> + struct vhost_virtqueue *tvq,
>>>>>> + bool rx)
>>>>>> +{
>>>>>> + struct socket *sock = rvq->private_data;
>>>>>> +
>>>>>> + if (rx)
>>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
>>>>>> + else if (sock && sk_has_rx_data(sock->sk))
>>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
>>>>>> + else {
>>>>>> + /* On tx here, sock has no rx data, so we
>>>>>> + * will wait for sock wakeup for rx, and
>>>>>> + * vhost_enable_notify() is not needed. */
>>>>> A possible case is we do have rx data but guest does not refill the rx
>>>>> queue. In this case we may lose notifications from guest.
>>>> Yes, should consider this case. thanks.
>>> I'm a bit confused. Isn't this covered by the previous
>>> "else if (sock && sk_has_rx_data(...))" block?
>> The problem is it does nothing if vhost_vq_avail_empty() is true and
>> vhost_enble_notify() is false.
> If vhost_enable_notify() is false, guest will eventually kicks vq, no?
>
Yes, you are right.
Thanks
^ permalink raw reply
* Re: linux-next: build failure after merge of the net-next tree
From: Jakub Kicinski @ 2018-08-03 2:34 UTC (permalink / raw)
To: Stephen Rothwell
Cc: David Miller, Networking, Linux-Next Mailing List,
Linux Kernel Mailing List
In-Reply-To: <20180803121430.25899c83@canb.auug.org.au>
On Fri, 3 Aug 2018 12:14:30 +1000, Stephen Rothwell wrote:
> Hi all,
>
> After merging the net-next tree, today's linux-next build (perf) failed
> like this:
>
> In file included from btf.c:10:0:
> btf.h:25:24: error: redundant redeclaration of 'btf__type_by_id' [-Werror=redundant-decls]
> const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 type_id);
> ^~~~~~~~~~~~~~~
> btf.h:20:24: note: previous declaration of 'btf__type_by_id' was here
> const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 id);
> ^~~~~~~~~~~~~~~
> btf.c:405:24: error: redefinition of 'btf__type_by_id'
> const struct btf_type *btf__type_by_id(const struct btf *btf,
> ^~~~~~~~~~~~~~~
> btf.c:197:24: note: previous definition of 'btf__type_by_id' was here
> const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 type_id)
> ^~~~~~~~~~~~~~~
> btf.c:40:20: error: 'btf_name_by_offset' defined but not used [-Werror=unused-function]
> static const char *btf_name_by_offset(const struct btf *btf, __u32 offset)
> ^~~~~~~~~~~~~~~~~~
> mv: cannot stat 'perf/.libbpf.o.tmp': No such file or directory
> tools/build/Makefile.build:96: recipe for target 'perf/libbpf.o' failed
> In file included from libbpf.c:52:0:
> btf.h:25:24: error: redundant redeclaration of 'btf__type_by_id' [-Werror=redundant-decls]
> const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 type_id);
> ^~~~~~~~~~~~~~~
> btf.h:20:24: note: previous declaration of 'btf__type_by_id' was here
> const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 id);
> ^~~~~~~~~~~~~~~
> mv: cannot stat 'perf/.libbpf.o.tmp': No such file or directory
> tools/build/Makefile.build:96: recipe for target 'perf/libbpf.o' failed
>
> Caused by commit
>
> 89b1698c93a9 ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/davem/net")
>
> Some of the lines in tools/lib/bpf/btf.[ch] were duplicated (they existed
> on both sides of the merge).
>
> I have used the net-next tree from next-20180802 for today.
I just noticed that too, fix out:
http://patchwork.ozlabs.org/patch/953018/
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Toshiaki Makita @ 2018-08-03 4:25 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: Linux Kernel Network Developers, virtualization, mst
In-Reply-To: <CAMDZJNUyZwcVpYc-3WGB_0CWSe-ti7c0BOvyFwwTo6PZ4s5V1Q@mail.gmail.com>
On 2018/08/03 13:14, Tonghao Zhang wrote:
> On Fri, Aug 3, 2018 at 11:40 AM Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp> wrote:
>>
>> On 2018/08/03 12:24, Tonghao Zhang wrote:
>>> On Fri, Aug 3, 2018 at 11:07 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2018年08月03日 10:51, Tonghao Zhang wrote:
>>>>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
>>>>>>> On 2018/08/02 17:18, Jason Wang wrote:
>>>>>>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
>>>>>>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>>>>>>>>>> + struct vhost_virtqueue *rvq,
>>>>>>>>>> + struct vhost_virtqueue *tvq,
>>>>>>>>>> + bool rx)
>>>>>>>>>> +{
>>>>>>>>>> + struct socket *sock = rvq->private_data;
>>>>>>>>>> +
>>>>>>>>>> + if (rx)
>>>>>>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
>>>>>>>>>> + else if (sock && sk_has_rx_data(sock->sk))
>>>>>>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
>>>>>>>>>> + else {
>>>>>>>>>> + /* On tx here, sock has no rx data, so we
>>>>>>>>>> + * will wait for sock wakeup for rx, and
>>>>>>>>>> + * vhost_enable_notify() is not needed. */
>>>>>>>>> A possible case is we do have rx data but guest does not refill the rx
>>>>>>>>> queue. In this case we may lose notifications from guest.
>>>>>>>> Yes, should consider this case. thanks.
>>>>>>> I'm a bit confused. Isn't this covered by the previous
>>>>>>> "else if (sock && sk_has_rx_data(...))" block?
>>>>>> The problem is it does nothing if vhost_vq_avail_empty() is true and
>>>>>> vhost_enble_notify() is false.
>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> + cpu_relax();
>>>>>>>>>>> + }
>>>>>>>>>>> +
>>>>>>>>>>> + preempt_enable();
>>>>>>>>>>> +
>>>>>>>>>>> + if (!rx)
>>>>>>>>>>> + vhost_net_enable_vq(net, vq);
>>>>>>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
>>>>>>>>>> called soon.
>>>>>>>>> If we disable rx virtqueue in handle_tx and don't send packets from
>>>>>>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
>>>>>>>>> so the network is broken.
>>>>>>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
>>>>>>>> there's no need to enable it since handle_rx() will do this for us.
>>>>>>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
>>>>>>> need to enable vq since in that case we have no rx data and handle_rx()
>>>>>>> is not scheduled.
>>>>>>>
>>>>>> Yes.
>>>>> So we will use the vhost_has_work() to check whether or not the
>>>>> handle_rx is scheduled ?
>>>>> If we use the vhost_has_work(), the work in the dev work_list may be
>>>>> rx work, or tx work, right ?
>>>>
>>>> Yes. We can add a boolean to record whether or not we've called
>>>> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
>>>> it was true.
>>> so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypoll"
>>> may not consider the case: work is tx work in the dev work list.
>>
>> Not sure what you are concerned but what I can say is that we need to
>> poll rx work if vhost_has_work() detects tx work in
>> vhost_net_rx_peek_head_len() since rx busypoll exits prematurely in that
>> case.
> In the handle_rx, when we busy poll, the vhost_has_work() return true,
> because the tx but not rx work is in the dev work list.
> and it is the most case, because tx work may be added to dev work list
> anytime(not during busy poll) when guest kick the vhost-net.
> so it is not necessary to add it., right ?
I'm lost.
What is the part you think is not needed?
1. When there is a tx work we exit rx busypoll.
2. When we exit rx busypoll by tx work, we poll rx work (so that we can
continue rx busypoll later).
--
Toshiaki Makita
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH rdma-next 00/10] IPoIB uninit
From: Jason Gunthorpe @ 2018-08-03 2:31 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Denis Drozdov,
Erez Shitrit, Saeed Mahameed, linux-netdev
In-Reply-To: <20180729083500.5352-1-leon@kernel.org>
On Sun, Jul 29, 2018 at 11:34:50AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> IP link was broken due to the changes in IPoIB for the rdma_netdev
> support after commit cd565b4b51e5 ("IB/IPoIB: Support acceleration options callbacks").
>
> This patchset restores IPoIB pkey creation and removal using rtnetlink.
>
> It is completely rewritten variant of
> https://marc.info/?l=linux-rdma&m=151553425830918&w=2 patch series.
>
> Thanks
>
> Erez Shitrit (2):
> IB/ipoib: Use cancel_delayed_work_sync for neigh-clean task
> IB/ipoib: Make ipoib_neigh_hash_uninit fully fence its work
>
> Jason Gunthorpe (8):
> IB/ipoib: Get rid of IPOIB_FLAG_GOING_DOWN
> IB/ipoib: Move all uninit code into ndo_uninit
> IB/ipoib: Move init code to ndo_init
> RDMA/netdev: Use priv_destructor for netdev cleanup
> IB/ipoib: Get rid of the sysfs_mutex
> IB/ipoib: Do not remove child devices from within the ndo_uninit
> IB/ipoib: Maintain the child_intfs list from ndo_init/uninit
> IB/ipoib: Consolidate checking of the proposed child interface
Applied to for-next, finally this bug is fixed..
I squashed these two patches though:
IB/ipoib: Use cancel_delayed_work_sync for neigh-clean task
IB/ipoib: Make ipoib_neigh_hash_uninit fully fence its work
Thanks,
Jason
^ permalink raw reply
* [PATCH net-next] tools: bpf: fix BTF code added twice to different trees
From: Jakub Kicinski @ 2018-08-03 2:30 UTC (permalink / raw)
To: davem
Cc: alexei.starovoitov, daniel, kafai, Okash Khawaja, oss-drivers,
netdev, Jakub Kicinski
commit 38d5d3b3d5db ("bpf: Introduce BPF_ANNOTATE_KV_PAIR")
added to the bpf and net trees what
commit 92b57121ca79 ("bpf: btf: export btf types and name by offset from lib")
has already added to bpf-next/net-next, but in slightly different
location. Remove the duplicates (to fix build of libbpf).
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
tools/lib/bpf/btf.c | 17 -----------------
tools/lib/bpf/btf.h | 1 -
2 files changed, 18 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 1622a309f169..09ecf8162f7a 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -37,14 +37,6 @@ struct btf {
int fd;
};
-static const char *btf_name_by_offset(const struct btf *btf, __u32 offset)
-{
- if (offset < btf->hdr->str_len)
- return &btf->strings[offset];
- else
- return NULL;
-}
-
static int btf_add_type(struct btf *btf, struct btf_type *t)
{
if (btf->types_size - btf->nr_types < 2) {
@@ -401,12 +393,3 @@ const char *btf__name_by_offset(const struct btf *btf, __u32 offset)
else
return NULL;
}
-
-const struct btf_type *btf__type_by_id(const struct btf *btf,
- __u32 type_id)
-{
- if (type_id > btf->nr_types)
- return NULL;
-
- return btf->types[type_id];
-}
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index dd8a86eab8ca..43c658ccfc2b 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -22,6 +22,5 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id);
int btf__resolve_type(const struct btf *btf, __u32 type_id);
int btf__fd(const struct btf *btf);
const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
-const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 type_id);
#endif
--
2.17.1
^ permalink raw reply related
* Re: [V9fs-developer] [PATCH] net/9p: Modify the problem of BUG_ON judgment
From: Dominique Martinet @ 2018-08-03 4:23 UTC (permalink / raw)
To: jiangyiwen
Cc: Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov,
Linux Kernel Mailing List, v9fs-developer, netdev
In-Reply-To: <5B63D5F6.6080109@huawei.com>
jiangyiwen wrote on Fri, Aug 03, 2018:
> Because the value of limit is VIRTQUEUE_NUM, if index is equal to
> limit, it will cause sg array out of bounds, so correct the judgement
> of BUG_ON.
>
> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
I'm not sure you've acted on his mail or if you found this
independantly, but this was reported by Dan Carpenter on the list in
June.
Would you mind if I add a tag for him?
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
That aside this looks good, I'll take it.
> ---
> net/9p/trans_virtio.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 6265d1d..08264ba 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -191,7 +191,7 @@ static int pack_sg_list(struct scatterlist *sg, int start,
> s = rest_of_page(data);
> if (s > count)
> s = count;
> - BUG_ON(index > limit);
> + BUG_ON(index >= limit);
> /* Make sure we don't terminate early. */
> sg_unmark_end(&sg[index]);
> sg_set_buf(&sg[index++], data, s);
> @@ -236,6 +236,7 @@ static int p9_virtio_cancel(struct p9_client *client, struct p9_req_t *req)
> s = PAGE_SIZE - data_off;
> if (s > count)
> s = count;
> + BUG_ON(index >= limit);
> /* Make sure we don't terminate early. */
> sg_unmark_end(&sg[index]);
> sg_set_page(&sg[index++], pdata[i++], s, data_off);
--
Dominique Martinet
^ permalink raw reply
* [V9fs-developer] [PATCH] net/9p: Modify the problem of BUG_ON judgment
From: jiangyiwen @ 2018-08-03 4:11 UTC (permalink / raw)
To: Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov,
Dominique Martinet
Cc: Linux Kernel Mailing List, v9fs-developer, netdev
Because the value of limit is VIRTQUEUE_NUM, if index is equal to
limit, it will cause sg array out of bounds, so correct the judgement
of BUG_ON.
Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
---
net/9p/trans_virtio.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 6265d1d..08264ba 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -191,7 +191,7 @@ static int pack_sg_list(struct scatterlist *sg, int start,
s = rest_of_page(data);
if (s > count)
s = count;
- BUG_ON(index > limit);
+ BUG_ON(index >= limit);
/* Make sure we don't terminate early. */
sg_unmark_end(&sg[index]);
sg_set_buf(&sg[index++], data, s);
@@ -236,6 +236,7 @@ static int p9_virtio_cancel(struct p9_client *client, struct p9_req_t *req)
s = PAGE_SIZE - data_off;
if (s > count)
s = count;
+ BUG_ON(index >= limit);
/* Make sure we don't terminate early. */
sg_unmark_end(&sg[index]);
sg_set_page(&sg[index++], pdata[i++], s, data_off);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Tonghao Zhang @ 2018-08-03 4:04 UTC (permalink / raw)
To: jasowang; +Cc: Linux Kernel Network Developers, mst, virtualization
In-Reply-To: <a900e9f3-3aad-2cfa-da4c-226bf8981059@redhat.com>
On Fri, Aug 3, 2018 at 11:43 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年08月03日 11:24, Tonghao Zhang wrote:
> > On Fri, Aug 3, 2018 at 11:07 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >>
> >> On 2018年08月03日 10:51, Tonghao Zhang wrote:
> >>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang <jasowang@redhat.com> wrote:
> >>>>
> >>>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
> >>>>> On 2018/08/02 17:18, Jason Wang wrote:
> >>>>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
> >>>>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
> >>>>>>>> + struct vhost_virtqueue *rvq,
> >>>>>>>> + struct vhost_virtqueue *tvq,
> >>>>>>>> + bool rx)
> >>>>>>>> +{
> >>>>>>>> + struct socket *sock = rvq->private_data;
> >>>>>>>> +
> >>>>>>>> + if (rx)
> >>>>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
> >>>>>>>> + else if (sock && sk_has_rx_data(sock->sk))
> >>>>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
> >>>>>>>> + else {
> >>>>>>>> + /* On tx here, sock has no rx data, so we
> >>>>>>>> + * will wait for sock wakeup for rx, and
> >>>>>>>> + * vhost_enable_notify() is not needed. */
> >>>>>>> A possible case is we do have rx data but guest does not refill the rx
> >>>>>>> queue. In this case we may lose notifications from guest.
> >>>>>> Yes, should consider this case. thanks.
> >>>>> I'm a bit confused. Isn't this covered by the previous
> >>>>> "else if (sock && sk_has_rx_data(...))" block?
> >>>> The problem is it does nothing if vhost_vq_avail_empty() is true and
> >>>> vhost_enble_notify() is false.
> >>>>
> >>>>>>>>> +
> >>>>>>>>> + cpu_relax();
> >>>>>>>>> + }
> >>>>>>>>> +
> >>>>>>>>> + preempt_enable();
> >>>>>>>>> +
> >>>>>>>>> + if (!rx)
> >>>>>>>>> + vhost_net_enable_vq(net, vq);
> >>>>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
> >>>>>>>> called soon.
> >>>>>>> If we disable rx virtqueue in handle_tx and don't send packets from
> >>>>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
> >>>>>>> so the network is broken.
> >>>>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
> >>>>>> there's no need to enable it since handle_rx() will do this for us.
> >>>>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
> >>>>> need to enable vq since in that case we have no rx data and handle_rx()
> >>>>> is not scheduled.
> >>>>>
> >>>> Yes.
> >>> So we will use the vhost_has_work() to check whether or not the
> >>> handle_rx is scheduled ?
> >>> If we use the vhost_has_work(), the work in the dev work_list may be
> >>> rx work, or tx work, right ?
> >> Yes. We can add a boolean to record whether or not we've called
> >> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
> >> it was true.
> > so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypoll"
> > may not consider the case: work is tx work in the dev work list.
>
> So two kinds of work, tx kick or tx wakeup.
>
> For tx kick, we check vhost_vq_avail_empty() and avoid unnecessary kicks
> by not enabling kick if we found something is pending on txq. For tx
> wakeup, yes, the commit does not consider it. And that's why we want to
> disable tx wakeups during busy polling.
And in the handle_rx but not busy polling, the tx can wakeup anytime
and the tx work will be added to dev work list. In that case, if we
add
the rx poll to the queue, it is necessary ？ the commit be294a51a may
check whether the rx work is in the dev work list.
> Thanks
>
> >
> >> So here's the needed changes:
> >>
> >> 1) Split the wakeup disabling to another patch
> >> 2) Squash the vhost_net_busy_poll_try_queue() and
> >> vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
> >> duplicated checks.
> >> 3) If possible, rename the boolean rx in vhost_net_busy_poll() to
> >> poll_rx, this makes code more readable.
> > OK
> >> Thanks
> >>
> >>>> Thanks
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Toshiaki Makita @ 2018-08-03 3:40 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: Linux Kernel Network Developers, virtualization, mst
In-Reply-To: <CAMDZJNWZ+WH3JhCy81h1VSka7-z2zF1Tw-EnLTr26J_5SkQZTg@mail.gmail.com>
On 2018/08/03 12:24, Tonghao Zhang wrote:
> On Fri, Aug 3, 2018 at 11:07 AM Jason Wang <jasowang@redhat.com> wrote:
>> On 2018年08月03日 10:51, Tonghao Zhang wrote:
>>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
>>>>> On 2018/08/02 17:18, Jason Wang wrote:
>>>>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
>>>>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>>>>>>>> + struct vhost_virtqueue *rvq,
>>>>>>>> + struct vhost_virtqueue *tvq,
>>>>>>>> + bool rx)
>>>>>>>> +{
>>>>>>>> + struct socket *sock = rvq->private_data;
>>>>>>>> +
>>>>>>>> + if (rx)
>>>>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
>>>>>>>> + else if (sock && sk_has_rx_data(sock->sk))
>>>>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
>>>>>>>> + else {
>>>>>>>> + /* On tx here, sock has no rx data, so we
>>>>>>>> + * will wait for sock wakeup for rx, and
>>>>>>>> + * vhost_enable_notify() is not needed. */
>>>>>>> A possible case is we do have rx data but guest does not refill the rx
>>>>>>> queue. In this case we may lose notifications from guest.
>>>>>> Yes, should consider this case. thanks.
>>>>> I'm a bit confused. Isn't this covered by the previous
>>>>> "else if (sock && sk_has_rx_data(...))" block?
>>>> The problem is it does nothing if vhost_vq_avail_empty() is true and
>>>> vhost_enble_notify() is false.
>>>>
>>>>>>>>> +
>>>>>>>>> + cpu_relax();
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + preempt_enable();
>>>>>>>>> +
>>>>>>>>> + if (!rx)
>>>>>>>>> + vhost_net_enable_vq(net, vq);
>>>>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
>>>>>>>> called soon.
>>>>>>> If we disable rx virtqueue in handle_tx and don't send packets from
>>>>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
>>>>>>> so the network is broken.
>>>>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
>>>>>> there's no need to enable it since handle_rx() will do this for us.
>>>>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
>>>>> need to enable vq since in that case we have no rx data and handle_rx()
>>>>> is not scheduled.
>>>>>
>>>> Yes.
>>> So we will use the vhost_has_work() to check whether or not the
>>> handle_rx is scheduled ?
>>> If we use the vhost_has_work(), the work in the dev work_list may be
>>> rx work, or tx work, right ?
>>
>> Yes. We can add a boolean to record whether or not we've called
>> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
>> it was true.
> so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypoll"
> may not consider the case: work is tx work in the dev work list.
Not sure what you are concerned but what I can say is that we need to
poll rx work if vhost_has_work() detects tx work in
vhost_net_rx_peek_head_len() since rx busypoll exits prematurely in that
case.
>> So here's the needed changes:
>>
>> 1) Split the wakeup disabling to another patch
>> 2) Squash the vhost_net_busy_poll_try_queue() and
>> vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
>> duplicated checks.
>> 3) If possible, rename the boolean rx in vhost_net_busy_poll() to
>> poll_rx, this makes code more readable.
> OK
>> Thanks
>>
>>>> Thanks
>>
>
>
--
Toshiaki Makita
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Toshiaki Makita @ 2018-08-03 3:32 UTC (permalink / raw)
To: Jason Wang, Tonghao Zhang
Cc: Linux Kernel Network Developers, virtualization, mst
In-Reply-To: <90c0deec-6a7b-9787-b62f-7ea76a5cbd7c@redhat.com>
On 2018/08/03 12:07, Jason Wang wrote:
> On 2018年08月02日 17:23, Jason Wang wrote:
>>>>>>>
>>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
>>>>>> called soon.
>>>>> If we disable rx virtqueue in handle_tx and don't send packets from
>>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
>>>>> so the network is broken.
>>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
>>>> there's no need to enable it since handle_rx() will do this for us.
>>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
>>> need to enable vq since in that case we have no rx data and handle_rx()
>>> is not scheduled.
>>>
> Rethink about this, looks not. We enable rx wakeups in this case, so if
> there's pending data, handle_rx() will be schedule after
> vhost_net_enable_vq().
You are right, but what I wanted to say is vhost_net_enable_vq() should
be needed (I was talking about what would happen if
vhost_net_enable_vq() were removed). Also, I think we should move
vhost_net_enable_vq() from vhost_net_busy_poll() to this last "else"
block because this is the case where rx wakeups is required.
Anyway this part will be refactored so let's see what this code will
look like in next version.
--
Toshiaki Makita
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* RE: [PATCH 1/1] selftest/net: fix FILE_SIZE for 32 bit architecture.
From: Maninder Singh @ 2018-08-03 3:31 UTC (permalink / raw)
To: Eric Dumazet, davem@davemloft.net, shuahkh@osg.samsung.com
Cc: netdev@vger.kernel.org, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, edumazet@google.com, PANKAJ MISHRA,
AMIT SAHRAWAT, Vaneet Narang
In-Reply-To: <4a412194-99aa-7969-54a4-727368fbf82c@gmail.com>
Hi,
>On 08/02/2018 03:31 AM, Maninder Singh wrote:
>> FILE_SZ is defined as (1UL << 35), it will overflow
>> for 32 bit system and logic will break.
>>
>> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
>> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
>> ---
>> tools/testing/selftests/net/tcp_mmap.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/tcp_mmap.c b/tools/testing/selftests/net/tcp_mmap.c
>> index e8c5dff..1d6ca12 100644
>> --- a/tools/testing/selftests/net/tcp_mmap.c
>> +++ b/tools/testing/selftests/net/tcp_mmap.c
>> @@ -85,7 +85,7 @@
>> #define MSG_ZEROCOPY 0x4000000
>> #endif
>>
>> -#define FILE_SZ (1UL << 35)
>> +#define FILE_SZ (1ULL << 35)
...
...
>> @@ -431,7 +431,7 @@ int main(int argc, char *argv[])
>> zflg = 0;
>> }
>> while (total < FILE_SZ) {
>> - long wr = FILE_SZ - total;
>> + unsigned long long wr = FILE_SZ - total;
>>
>> if (wr > chunk_size)
>> wr = chunk_size;
>>
>
>What about using more conventional size_t instead of "unsigned long long" ?
size_t is also equivalent to unsigned long and it will not hold value of (1 << 35) for 32 bit system.
So we can do two things.
(1) reduce FILE SIZE to (1 << 30), so that UL (size_t) can hold this value.
It will not show any perofrmance boost with ZEROCOPY. (checked on x86_64)
(2) use unsigned long long to work with both 32 and 64 bit system.
It will show performance boost with ZEROCOPY.(checked on x86_64)
What do you think?
Thanks and regards,
Maninder Singh
^ permalink raw reply
* RE: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
From: Vakul Garg @ 2018-08-03 1:23 UTC (permalink / raw)
To: Doron Roberts-Kedes, David S . Miller
Cc: Dave Watson, Boris Pismenny, Aviad Yehezkel,
netdev@vger.kernel.org
In-Reply-To: <20180803003001.1338460-1-doronrk@fb.com>
> -----Original Message-----
> From: Doron Roberts-Kedes [mailto:doronrk@fb.com]
> Sent: Friday, August 3, 2018 6:00 AM
> To: David S . Miller <davem@davemloft.net>
> Cc: Dave Watson <davejwatson@fb.com>; Vakul Garg
> <vakul.garg@nxp.com>; Boris Pismenny <borisp@mellanox.com>; Aviad
> Yehezkel <aviadye@mellanox.com>; netdev@vger.kernel.org; Doron
> Roberts-Kedes <doronrk@fb.com>
> Subject: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without
> skb_cow_data.
>
> decrypt_skb fails if the number of sg elements required to map is greater
> than MAX_SKB_FRAGS. As noted by Vakul Garg, nsg must always be
> calculated, but skb_cow_data adds unnecessary memcpy's for the zerocopy
> case.
>
> The new function skb_nsg calculates the number of scatterlist elements
> required to map the skb without the extra overhead of skb_cow_data. This
> function mimics the structure of skb_to_sgvec.
>
> Fixes: c46234ebb4d1 ("tls: RX path for ktls")
> Signed-off-by: Doron Roberts-Kedes <doronrk@fb.com>
> ---
> net/tls/tls_sw.c | 89
> ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 86 insertions(+), 3 deletions(-)
>
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index
> ff3a6904a722..c62793601cfc 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -43,6 +43,76 @@
>
> #define MAX_IV_SIZE TLS_CIPHER_AES_GCM_128_IV_SIZE
>
> +static int __skb_nsg(struct sk_buff *skb, int offset, int len,
> + unsigned int recursion_level)
> +{
> + int start = skb_headlen(skb);
> + int i, copy = start - offset;
> + struct sk_buff *frag_iter;
> + int elt = 0;
> +
> + if (unlikely(recursion_level >= 24))
> + return -EMSGSIZE;
> +
> + if (copy > 0) {
> + if (copy > len)
> + copy = len;
> + elt++;
> + if ((len -= copy) == 0)
> + return elt;
> + offset += copy;
> + }
> +
> + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> + int end;
> +
> + WARN_ON(start > offset + len);
> +
> + end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
> + if ((copy = end - offset) > 0) {
> + if (copy > len)
> + copy = len;
> + elt++;
> + if (!(len -= copy))
> + return elt;
> + offset += copy;
> + }
> + start = end;
> + }
> +
> + skb_walk_frags(skb, frag_iter) {
> + int end, ret;
> +
> + WARN_ON(start > offset + len);
> +
> + end = start + frag_iter->len;
> + if ((copy = end - offset) > 0) {
> +
> + if (copy > len)
> + copy = len;
> + ret = __skb_nsg(frag_iter, offset - start, copy,
> + recursion_level + 1);
> + if (unlikely(ret < 0))
> + return ret;
> + elt += ret;
> + if ((len -= copy) == 0)
> + return elt;
> + offset += copy;
> + }
> + start = end;
> + }
> + BUG_ON(len);
> + return elt;
> +}
> +
> +/* Return the number of scatterlist elements required to completely map
> +the
> + * skb, or -EMSGSIZE if the recursion depth is exceeded.
> + */
> +static int skb_nsg(struct sk_buff *skb, int offset, int len) {
> + return __skb_nsg(skb, offset, len, 0); }
> +
These is generic function and useful elsewhere too.
Should the above two functions be exported by skbuff.c?
> static int tls_do_decryption(struct sock *sk,
> struct scatterlist *sgin,
> struct scatterlist *sgout,
> @@ -693,7 +763,7 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb,
> struct scatterlist sgin_arr[MAX_SKB_FRAGS + 2];
> struct scatterlist *sgin = &sgin_arr[0];
> struct strp_msg *rxm = strp_msg(skb);
> - int ret, nsg = ARRAY_SIZE(sgin_arr);
> + int ret, nsg;
> struct sk_buff *unused;
>
> ret = skb_copy_bits(skb, rxm->offset + TLS_HEADER_SIZE, @@ -
> 704,10 +774,23 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb,
>
> memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
> if (!sgout) {
> - nsg = skb_cow_data(skb, 0, &unused) + 1;
> + nsg = skb_cow_data(skb, 0, &unused);
> + } else {
> + nsg = skb_nsg(skb,
> + rxm->offset + tls_ctx->rx.prepend_size,
> + rxm->full_len - tls_ctx->rx.prepend_size);
> + if (nsg <= 0)
> + return nsg;
Comparison should be (nsg < 1). TLS forbids '0' sized records.
> + }
> +
> + // We need one extra for ctx->rx_aad_ciphertext
> + nsg++;
> +
> + if (nsg > ARRAY_SIZE(sgin_arr))
> sgin = kmalloc_array(nsg, sizeof(*sgin), sk->sk_allocation);
Add check for kmalloc_array returnining NULL.
> +
> + if (!sgout)
> sgout = sgin;
> - }
>
> sg_init_table(sgin, nsg);
> sg_set_buf(&sgin[0], ctx->rx_aad_ciphertext, TLS_AAD_SPACE_SIZE);
> --
> 2.17.1
^ permalink raw reply
* Re: [PATCH] ppp: mppe: Remove VLA usage
From: Paul Mackerras @ 2018-08-03 3:13 UTC (permalink / raw)
To: Kees Cook
Cc: Arnd Bergmann, David S. Miller, Herbert Xu, Eric Biggers,
Gustavo A. R. Silva, linux-ppp, Networking,
Linux Kernel Mailing List
In-Reply-To: <CAGXu5jKaNk8x9v+uVQ_O7gxr50s1-Q901JbQupnkBFYiUjd7UQ@mail.gmail.com>
On Thu, Aug 02, 2018 at 04:27:01PM -0700, Kees Cook wrote:
> On Mon, Jul 16, 2018 at 4:01 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Jul 16, 2018 at 6:05 AM, Kees Cook <keescook@chromium.org> wrote:
> >> In the quest to remove all stack VLA usage from the kernel[1], this
> >> removes the discouraged use of AHASH_REQUEST_ON_STACK (and associated
> >> VLA) by switching to shash directly and keeping the associated descriptor
> >> allocated with the regular state on the heap.
> >>
> >> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > I had concerns at first that this approach might make it slower, but
> > upon reading through implementation of the shash_ahash_ implementation,
> > I concluded that it can only be better than before, improving both
> > performance and stack usage.
> >
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> Paul,
>
> Is this something you can take via your tree?
No, it would go via the netdev tree, I expect.
Paul.
^ permalink raw reply
* Re: [PATCH v1 2/3] zinc: Introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-08-03 2:48 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Eric Biggers, Linux Crypto Mailing List, LKML, Netdev,
David Miller, Andrew Lutomirski, Greg Kroah-Hartman, Samuel Neves,
Daniel J . Bernstein, Tanja Lange, Jean-Philippe Aumasson,
Karthikeyan Bhargavan
In-Reply-To: <CALCETrXD_VyoyW5C1U34o-ySJ4nFcO6PQ+ZNBVYxCpJ-f65CHw@mail.gmail.com>
Hey Andy,
Thanks too for the feedback. Responses below:
On Wed, Aug 1, 2018 at 7:09 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > I think the above changes would also naturally lead to a much saner
> > patch series where each algorithm is added by its own patch, rather than
> > one monster patch that adds many algorithms and 24000 lines of code.
> >
>
> Yes, please.
Ack, will be in v2.
> I like this a *lot*. (But why are you passing have_simd? Shouldn't
> that check live in chacha20_arch? If there's some init code needed,
> then chacha20_arch() should just return false before the init code
> runs. Once the arch does whatever feature detection it needs, it can
> make chacha20_arch() start returning true.)
The have_simd stuff is so that the FPU state can be amortized across
several calls to the crypto functions. Here's a snippet from
WireGuard's send.c:
void packet_encrypt_worker(struct work_struct *work)
{
struct crypt_queue *queue = container_of(work, struct
multicore_worker, work)->ptr;
struct sk_buff *first, *skb, *next;
bool have_simd = simd_get();
while ((first = ptr_ring_consume_bh(&queue->ring)) != NULL) {
enum packet_state state = PACKET_STATE_CRYPTED;
skb_walk_null_queue_safe(first, skb, next) {
if (likely(skb_encrypt(skb, PACKET_CB(first)->keypair, have_simd)))
skb_reset(skb);
else {
state = PACKET_STATE_DEAD;
break;
}
}
queue_enqueue_per_peer(&PACKET_PEER(first)->tx_queue, first, state);
have_simd = simd_relax(have_simd);
}
simd_put(have_simd);
}
simd_get() and simd_put() do the usual irq_fpu_usable/kernel_fpu_begin
dance and return/take a boolean accordingly. simd_relax(on) is:
static inline bool simd_relax(bool was_on)
{
#ifdef CONFIG_PREEMPT
if (was_on && need_resched()) {
simd_put(true);
return simd_get();
}
#endif
return was_on;
}
With this, we most of the time get the FPU amortization, while still
doing the right thing for the preemption case (since kernel_fpu_begin
disables preemption). This is a quite important performance
optimization. However, I'd prefer the lazy FPU restoration proposal
discussed a few months ago, but it looks like that hasn't progressed,
hence the need for FPU call amortization:
https://lore.kernel.org/lkml/CALCETrU+2mBPDfkBz1i_GT1EOJau+mzj4yOK8N0UnT2pGjiUWQ@mail.gmail.com/
>
> As I see it, there there are two truly new thing in the zinc patchset:
> the direct (in the direct call sense) arch dispatch, and the fact that
> the functions can be called directly, without allocating contexts,
> using function pointers, etc.
>
> In fact, I had a previous patch set that added such an interface for SHA256.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=8c59a4dd8b7ba4f2e5a6461132bbd16c83ff7c1f
>
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=7e5fbc02972b03727b71bc71f84175c36cbf01f5
Seems like SHA256 will be a natural next candidate for Zinc, given the demand.
> > Your patch description is also missing any mention of crypto accelerator
> > hardware. Quite a bit of the complexity in the crypto API, such as
> > scatterlist support and asynchronous execution, exists because it
> > supports crypto accelerators. AFAICS your new APIs cannot support
> > crypto accelerators, as your APIs are synchronous and operate on virtual
> > addresses. I assume your justification is that "djb algorithms" like
> > ChaCha and Poly1305 don't need crypto accelerators as they are fast in
> > software. But you never explicitly stated this and discussed the
> > tradeoffs. Since this is basically the foundation for the design you've
> > chosen, it really needs to be addressed.
>
> I see this as an advantage, not a disadvantage. A very large majority
> of in-kernel crypto users (by number of call sites under a *very*
> brief survey, not by number of CPU cycles) just want to do some
> synchronous crypto on a buffer that is addressed by a regular pointer.
> Most of these users would be slowed down if they used any form of
> async crypto, since the CPU can complete the whole operation faster
> than it could plausibly initiate and complete anything asynchronous.
> And, right now, they suffer the full overhead of allocating a context
> (often with alloca!), looking up (or caching) some crypto API data
> structures, dispatching the operation, and cleaning up.
>
> So I think the right way to do it is to have directly callable
> functions like zinc uses and to have the fancy crypto API layer on top
> of them. So if you actually want async accelerated crypto with
> scatterlists or whatever, you can call into the fancy API, and the
> fancy API can dispatch to hardware or it can dispatch to the normal
> static API.
Yes, exactly this.
Regards,
Jason
^ permalink raw reply
* Re: linux-next: build failure after merge of the net-next tree
From: David Miller @ 2018-08-03 2:44 UTC (permalink / raw)
To: sfr; +Cc: netdev, linux-next, linux-kernel
In-Reply-To: <20180803121430.25899c83@canb.auug.org.au>
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Fri, 3 Aug 2018 12:14:30 +1000
> Hi all,
>
> After merging the net-next tree, today's linux-next build (perf) failed
> like this:
>
> In file included from btf.c:10:0:
...
> Caused by commit
>
> 89b1698c93a9 ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/davem/net")
>
> Some of the lines in tools/lib/bpf/btf.[ch] were duplicated (they existed
> on both sides of the merge).
>
> I have used the net-next tree from next-20180802 for today.
My bad, I am working on fixing this now.
Thanks.
^ permalink raw reply
* Re: [PATCH v1 2/3] zinc: Introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-08-03 2:33 UTC (permalink / raw)
To: Eric Biggers
Cc: Linux Crypto Mailing List, LKML, Netdev, David Miller,
Andrew Lutomirski, Greg Kroah-Hartman, Samuel Neves,
Daniel J . Bernstein, Tanja Lange, Jean-Philippe Aumasson,
Karthikeyan Bhargavan
In-Reply-To: <20180801072246.GA15677@sol.localdomain>
Hi Eric,
Thanks for your feedback. Responses below.
On Wed, Aug 1, 2018 at 9:22 AM Eric Biggers <ebiggers3@gmail.com> wrote:
> it's intended that the "Zinc" algorithms will be exposed through the
> crypto API -- just like how most of the existing crypto code in lib/ is
> also exposed through the crypto API. So, I think that what you're doing
> isn't actually *that* different from what already exists in some cases;
> and pretending that it is very different is just going to cause
> problems.
I think as an eventual thing, it'd be great to see the whole of the
software part of the Crypto API rebased ontop of Zinc's software
implementations. But the Cryto API is huge, and it's not really
realistic to do this all at once. I'd also like to have a bit more
scrutiny of each individual Zinc submission, rather than just dumping
it all in there wholesale. Additionally, it seems reasonable to do
this in accordance with actual need for using the algorithms from
Zinc, i.e., not rushing toward DES.
> Rather, the actual truly new thing seems to be that the [...]
> [...] are part of some holy crusade against the crypto API.
> So much for WireGuard being only
I sense a bit of snark in your comments, and I am sorry if I ruffled
some feathers. I can understand a number of things might have rubbed
you the wrong way. But I am pretty committed to getting this done
right, taking into account your and Andy's opinions, as well as
working hard to keep Zinc close to its design goals. I think you'll
find we're closer to being on the same page than not, and that I only
want to make things better rather than be on some kind of "crusade"
against anything.
> They could even still go in subdirectory lib/crypto/ -- but just for
> logical code organization purposes,
Indeed a subdirectory is important for organization. We can bikeshed
about names, I suppose. I'm partial zinc, and I think that it might
actually help foster contributors and interested academics. Anyway,
let's dive into the code issues first, though, as I think that'll be a
productive way to get going.
> CONFIG_ZINC also needs to go. Algorithms will need to be independently
> configurable as soon as anything other than WireGuard needs to use any
> of them, so you might as well do it right from the start with
> CONFIG_BLAKE2, CONFIG_CHACHA20, CONFIG_POLY1305, etc.
Okay, I can add this all as individual nobs. I'll do that for v2.
I'm tempted to still have this built as one module, though. For
example, should WireGuard depend on 5 mini-modules (blake2s, chacha20,
poly1305, chacha20poly1305, curve25519)? Rather, it seems a bit
sleeker to just have a single zinc.ko, and allow what's built into it
be configurable with the various algorithm configuration options. What
would you think of this? Or do you think you'd still prefer
one-algo-per-ko approach like with the current crypto API? I can see
good arguments both ways.
> I think the above changes would also naturally lead to a much saner
> patch series where each algorithm is added by its own patch, rather than
> one monster patch that adds many algorithms and 24000 lines of code.
Yes, absolutely! And sorry for that monster patch; it even got
rejected from LKML as you pointed out. I had started to split things
up, but I wanted to send the patchset foremost to netdev, to start
getting some feedback on WireGuard, and wanted to keep the
organization simple. I'll have this split up for v2.
>
> Note that adding all the algorithms in one patch also makes the
> description of them conflated, e.g. you wrote that "formally verified"
> implementations were used whenever possible, but AFAICS that actually
> only applies to the C implementations of Curve25519,
Yes, the commit message mentioned which implementations have been
verified. It's quite likely that in the near future we'll be adding
more formally verified implementations, and even formally verified
assembly implementations. While BLAKE2s and ChaCha20 are somewhat
straight forward, bigint stuff, such as Curve25519 and Poly1305,
always make me a bit nervous, and so we'll probably be additionally
incorporating a formally verified Poly1305 first of the bunch.
Either way, _all_ of those implementations have been reviewed in some
depth, fuzzed absurd amounts, and fed some pretty nasty test vectors.
I've spent a lot of time with all of them, and most have received a
good amount of external scrutiny, due to their origin.
> So, I'd strongly prefer that you don't oversell the crypto
> code you're adding, e.g. by implying that most of it is formally
> verified, as it likely still has bugs, like any other code...
This sounds like snark that's going to devolve into endless
bikeshedding, but I do note that I mentioned formally verified
implementations in these 3 contexts, which I think were appropriate
each time:
1) "Wherever possible and performant, formally verified
implementations are used, such as those from HACL* and Fiat-Crypto."
So, trying to state that we use it when we can, but we can't always. I
think it's important to mention it here, as already, I've been
contacted by formal verification people who are already eager to help.
This is exactly the kind of positive outcome I was hoping for.
2) "Curve25519: formally verified 64-bit C, formally verified 32-bit C, [...]"
Pointing out that these 2 implementations, in contradistinction to the
subsequent ones listed, are from formally verified C.
3) "potentially replacing them with either formally-verified
implementations [...]"
Again here, I indicate that I'd like to replace non formally-verified
implementations with formally-verified implementations wherever
there's such a potentiality. And like the previous comment, I believe
this has been interpreted as a welcome call to action by the people
who are actually working on this stuff.
So I don't think I'm "over selling" it. Formal verification is a good
thing. We have a little bit of it in Zinc. I'd like to have a lot more
of it. And all the while we're doing the best with what we've got.
Generally speaking, I believe the choices I've made on the individual
implementations in this submission have been made with a lot of care,
and I'm happy to work with you on any concerns you have about them and
do what's needed to get them in the proper shape.
> I also want to compare the performance of some of the assembly
> implementations you're adding to the existing implementations in the
> kernel they duplicate. I'm especially interested in the NEON
> implementation of ChaCha20. But adding 11 implementations in one single
> patch means there isn't really a chance to comment on them individually.
As noted, I'll certainly split this up for v2. With ChaCha20, my
benchmarks have indicated Zinc is faster than the Crypto API's
implementations. But there's also another benefit: the ChaCha20 and
Poly1305 implementations are based on Andy Polyakov's ones, which
means they benefit from the *huge* amount of scrutiny those have
already received and continue to receive. In fact, there are people
working on doing formally verified reproductions of some of his
implementations, and that alone is hugely valuable. The
implementations generally found in Linux haven't received nearly as
much scrutiny and attention, putting them at a bit of a disadvantage,
I think. In other words, there's a strong benefit from working
together and sharing in the enterprise with other projects.
There's another plus, too, which is that they all have a very common
interface, making the glue code simpler and more uniform. This is
probably mostly just a benefit for me, as a lazy coder who would
prefer an easier job, but usually less complexity and fewer
differences results in fewer bugs, too.
> Also, earlier when I tested OpenSSL's ChaCha NEON implementation on ARM
> Cortex-A7 it was actually quite a bit slower than the one in the Linux
> kernel written by Ard Biesheuvel... I trust that when claiming the
> performance of all implementations you're adding is "state-of-the-art
> and unrivaled", you actually compared them to the ones already in the
> Linux kernel which you're advocating replacing, right? :-)
Yes, I have, and my results don't corroborate your findings. It will
be interesting to get out a wider variety of hardware for comparisons.
I suspect, also, that if the snarky emoticons subside, AndyP would be
very interested in whatever we find and could have interest in
improving implementations, should we actually find performance
differences.
> Your patch description is also missing any mention of crypto accelerator
> hardware. Quite a bit of the complexity in the crypto API, such as
> scatterlist support and asynchronous execution, exists because it
> supports crypto accelerators.
I can augment that description, if you like, for v2. But with regards
to crypto accelerators -- I think the asynchronous API causes all
sorts of problems for some of the most common use cases, and so Zinc
is a software-based ordinary synchronous API. I think this is an
advantage. Of course you can continue to support wild async IPSec
accelerator hardware and such in the crypto API, but that's
intentionally not the focus of Zinc.
> I assume your justification is that "djb algorithms" like
> But you never explicitly stated this and discussed the
> tradeoffs.
It was already a pretty huge wall of text in the monster patch, but
even so there's probably tons of discussion about different details,
both large and small, that I must have left out. Sorry about that.
Hopefully these discussions here can help fill them in.
> The main problem, though, is that your code is a mess due to all
> the #ifdefs, and it will only get worse as people add more
> architectures.
I really thought the ifdefs were cleaner than the three other ways I
have implemented for doing it. When I showed this all to Greg KH
several months ago, he also thought the ifdefs were disgusting. It
seems you agree with him, and I'll certainly change this for v2. I
already have most of that code written, and based on your comments and
suggestions below, I'm sure you'll find it much better.
> Note that this
> would make the code much more consistent with the usual Linux kernel
> coding style, which strongly prefers calling functions unconditionally
> rather than having core logic littered with unmaintainable #ifdefs.
Noted. Will be changed.
> I'd also strongly prefer the patchset to include converting the crypto
> API versions of ChaCha20 and Poly1305 over to use your new lib/ code, to
> show that it's really possible. You mentioned that it's planned, but if
> it's not done right away there will be things that were missed and will
> require changes when someone finally does it. IMO it's not acceptable
> to add your own completely separate ChaCha20 and Poly1305 just because
> you don't like that the existing ones are part of the crypto API.
Alright. I didn't really want to do this right off the bat, but you
make a good argument that doing so is important in working out the
required details of just knowing how it's done. So I'll take a stab at
it for v2. I'll let you know if I run into any snags; we might be able
to collaborate nicely here.
> You
> need to refactor things properly. I think you'd need to expose the new
> code under a cra_driver_name like "chacha20-software" and
> "poly1305-software" to reflect that they use the fastest available
> implementation of the algorithm on the CPU, e.g. "chacha20-generic",
> "chacha20-neon", and "chacha20-simd" would all be replaced by a single
> "chacha20-software". Is that what you had in mind?
Calling it "chacha20-software" sounds like a reasonable way to name it.
>
> I'm also wondering about the origin and licensing of some of the
> assembly language files. Many have an OpenSSL copyright statement.
> But, the OpenSSL license is often thought to be incompatible with GPL,
> so using OpenSSL assembly code in the kernel has in the past required
> getting special permission from Andy Polyakov (the person who's written
> most of OpenSSL's assembly code so holds the copyright on it). As one
> example, see arch/arm/crypto/sha256-armv4.pl: the file explicitly states
> that Andy has relicensed it under GPLv2. For your new OpenSSL-derived
> files, have you gone through and explicitly gotten GPLv2 permission from
> Andy / the copyright holders?
Andy and I spoke in January about this and he was okay with the usual
CRYPTOGAMS GPLv2 origin dance for that code.
> For example lib/zinc/curve25519/curve25519-arm.S has your
> copyright statement and says it's "Based on algorithms from Daniel J.
> Bernstein and Peter Schwabe.", but it's not clarified whether the *code*
> was written by those other people, as opposed to those people designing
> the *algorithms* and then you writing the code; and if you didn't write
> it, where you retrieved the file from and when, what license it had
> (even if it was "public domain" like some of djb's code, this should be
> mentioned for informational purposes), and what changes you made if any.
It's indeed based on their public domain code. I'll change that
wording to make it very clear that it's from public domain code.
> Oh, and please use 80-character lines like the rest of the kernel, so
> that people's eyes don't bleed when reading your code :-)
Ack. Will be so in v2.
>
> Thanks for all your hard work on WireGuard!
Thanks for your excellent review. I really appreciate it you taking
the time. Hopefully v2 will start looking a lot more interesting to
you!
Regards,
Jason
^ permalink raw reply
* [PATCH net-next] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
From: Doron Roberts-Kedes @ 2018-08-03 0:30 UTC (permalink / raw)
To: David S . Miller
Cc: Dave Watson, Vakul Garg, Boris Pismenny, Aviad Yehezkel, netdev,
Doron Roberts-Kedes
decrypt_skb fails if the number of sg elements required to map is
greater than MAX_SKB_FRAGS. As noted by Vakul Garg, nsg must always be
calculated, but skb_cow_data adds unnecessary memcpy's for the zerocopy
case.
The new function skb_nsg calculates the number of scatterlist elements
required to map the skb without the extra overhead of skb_cow_data. This
function mimics the structure of skb_to_sgvec.
Fixes: c46234ebb4d1 ("tls: RX path for ktls")
Signed-off-by: Doron Roberts-Kedes <doronrk@fb.com>
---
net/tls/tls_sw.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 86 insertions(+), 3 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index ff3a6904a722..c62793601cfc 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -43,6 +43,76 @@
#define MAX_IV_SIZE TLS_CIPHER_AES_GCM_128_IV_SIZE
+static int __skb_nsg(struct sk_buff *skb, int offset, int len,
+ unsigned int recursion_level)
+{
+ int start = skb_headlen(skb);
+ int i, copy = start - offset;
+ struct sk_buff *frag_iter;
+ int elt = 0;
+
+ if (unlikely(recursion_level >= 24))
+ return -EMSGSIZE;
+
+ if (copy > 0) {
+ if (copy > len)
+ copy = len;
+ elt++;
+ if ((len -= copy) == 0)
+ return elt;
+ offset += copy;
+ }
+
+ for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+ int end;
+
+ WARN_ON(start > offset + len);
+
+ end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
+ if ((copy = end - offset) > 0) {
+ if (copy > len)
+ copy = len;
+ elt++;
+ if (!(len -= copy))
+ return elt;
+ offset += copy;
+ }
+ start = end;
+ }
+
+ skb_walk_frags(skb, frag_iter) {
+ int end, ret;
+
+ WARN_ON(start > offset + len);
+
+ end = start + frag_iter->len;
+ if ((copy = end - offset) > 0) {
+
+ if (copy > len)
+ copy = len;
+ ret = __skb_nsg(frag_iter, offset - start, copy,
+ recursion_level + 1);
+ if (unlikely(ret < 0))
+ return ret;
+ elt += ret;
+ if ((len -= copy) == 0)
+ return elt;
+ offset += copy;
+ }
+ start = end;
+ }
+ BUG_ON(len);
+ return elt;
+}
+
+/* Return the number of scatterlist elements required to completely map the
+ * skb, or -EMSGSIZE if the recursion depth is exceeded.
+ */
+static int skb_nsg(struct sk_buff *skb, int offset, int len)
+{
+ return __skb_nsg(skb, offset, len, 0);
+}
+
static int tls_do_decryption(struct sock *sk,
struct scatterlist *sgin,
struct scatterlist *sgout,
@@ -693,7 +763,7 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb,
struct scatterlist sgin_arr[MAX_SKB_FRAGS + 2];
struct scatterlist *sgin = &sgin_arr[0];
struct strp_msg *rxm = strp_msg(skb);
- int ret, nsg = ARRAY_SIZE(sgin_arr);
+ int ret, nsg;
struct sk_buff *unused;
ret = skb_copy_bits(skb, rxm->offset + TLS_HEADER_SIZE,
@@ -704,10 +774,23 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb,
memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
if (!sgout) {
- nsg = skb_cow_data(skb, 0, &unused) + 1;
+ nsg = skb_cow_data(skb, 0, &unused);
+ } else {
+ nsg = skb_nsg(skb,
+ rxm->offset + tls_ctx->rx.prepend_size,
+ rxm->full_len - tls_ctx->rx.prepend_size);
+ if (nsg <= 0)
+ return nsg;
+ }
+
+ // We need one extra for ctx->rx_aad_ciphertext
+ nsg++;
+
+ if (nsg > ARRAY_SIZE(sgin_arr))
sgin = kmalloc_array(nsg, sizeof(*sgin), sk->sk_allocation);
+
+ if (!sgout)
sgout = sgin;
- }
sg_init_table(sgin, nsg);
sg_set_buf(&sgin[0], ctx->rx_aad_ciphertext, TLS_AAD_SPACE_SIZE);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next] net/tls: Always get number of sg entries for skb to be decrypted
From: Doron Roberts-Kedes @ 2018-08-03 0:26 UTC (permalink / raw)
To: Dave Watson; +Cc: Vakul Garg, netdev, borisp, aviadye, davem
In-Reply-To: <20180802165058.GB66257@sharrissfo-mbp.dhcp.thefacebook.com>
On Thu, Aug 02, 2018 at 09:50:58AM -0700, Dave Watson wrote:
> On 08/02/18 09:50 PM, Vakul Garg wrote:
> > Function decrypt_skb() made a bad assumption that number of sg entries
> > required for mapping skb to be decrypted would always be less than
> > MAX_SKB_FRAGS. The required count of sg entries for skb should always be
> > calculated. If they cannot fit in local array sgin_arr[], allocate them
> > from heap irrespective whether it is zero-copy case or otherwise. The
> > change also benefits the non-zero copy case as we could use sgin_arr[]
> > instead of always allocating sg entries from heap.
> >
> > Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
>
> Looks great, thanks.
>
> Reviewed-by: Dave Waston <davejwatson@fb.com>
I agree that this is a problem, but I'm not sure that this is the best
way to fix it. Calling skb_cow_data unconditionally is expensive. In my
benchmarks this patch cause a 5% CPU regression, all from memcpy from
skb_cow_data, and a 15% regression in encrypted NBD IOPS. It is possible
to calculate the number of scatterlist elements required to map the skb
without invoking skb_cow_data. I'll have a patch up shortly.
^ permalink raw reply
* [PATCH net-next v3] ipv6: defrag: drop non-last frags smaller than min mtu
From: Florian Westphal @ 2018-08-03 0:22 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal, Peter Oskolkov, Eric Dumazet
don't bother with pathological cases, they only waste cycles.
IPv6 requires a minimum MTU of 1280 so we should never see fragments
smaller than this (except last frag).
v3: don't use awkward "-offset + len"
v2: drop IPv4 part, which added same check w. IPV4_MIN_MTU (68).
There were concerns that there could be even smaller frags
generated by intermediate nodes, e.g. on radio networks.
Cc: Peter Oskolkov <posk@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/ipv6/netfilter/nf_conntrack_reasm.c | 4 ++++
net/ipv6/reassembly.c | 4 ++++
2 files changed, 8 insertions(+)
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 0610bdab721c..aed9766559c9 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -557,6 +557,10 @@ int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
hdr = ipv6_hdr(skb);
fhdr = (struct frag_hdr *)skb_transport_header(skb);
+ if (skb->len - skb_network_offset(skb) < IPV6_MIN_MTU &&
+ fhdr->frag_off & htons(IP6_MF))
+ return -EINVAL;
+
skb_orphan(skb);
fq = fq_find(net, fhdr->identification, user, hdr,
skb->dev ? skb->dev->ifindex : 0);
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 6edd2ac8ae4b..a976b143463b 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -455,6 +455,10 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
return 1;
}
+ if (skb->len - skb_network_offset(skb) < IPV6_MIN_MTU &&
+ fhdr->frag_off & htons(IP6_MF))
+ goto fail_hdr;
+
iif = skb->dev ? skb->dev->ifindex : 0;
fq = fq_find(net, fhdr->identification, hdr, iif);
if (fq) {
--
2.16.4
^ permalink raw reply related
* linux-next: build failure after merge of the net-next tree
From: Stephen Rothwell @ 2018-08-03 2:14 UTC (permalink / raw)
To: David Miller, Networking
Cc: Linux-Next Mailing List, Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 2107 bytes --]
Hi all,
After merging the net-next tree, today's linux-next build (perf) failed
like this:
In file included from btf.c:10:0:
btf.h:25:24: error: redundant redeclaration of 'btf__type_by_id' [-Werror=redundant-decls]
const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 type_id);
^~~~~~~~~~~~~~~
btf.h:20:24: note: previous declaration of 'btf__type_by_id' was here
const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 id);
^~~~~~~~~~~~~~~
btf.c:405:24: error: redefinition of 'btf__type_by_id'
const struct btf_type *btf__type_by_id(const struct btf *btf,
^~~~~~~~~~~~~~~
btf.c:197:24: note: previous definition of 'btf__type_by_id' was here
const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 type_id)
^~~~~~~~~~~~~~~
btf.c:40:20: error: 'btf_name_by_offset' defined but not used [-Werror=unused-function]
static const char *btf_name_by_offset(const struct btf *btf, __u32 offset)
^~~~~~~~~~~~~~~~~~
mv: cannot stat 'perf/.libbpf.o.tmp': No such file or directory
tools/build/Makefile.build:96: recipe for target 'perf/libbpf.o' failed
In file included from libbpf.c:52:0:
btf.h:25:24: error: redundant redeclaration of 'btf__type_by_id' [-Werror=redundant-decls]
const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 type_id);
^~~~~~~~~~~~~~~
btf.h:20:24: note: previous declaration of 'btf__type_by_id' was here
const struct btf_type *btf__type_by_id(const struct btf *btf, __u32 id);
^~~~~~~~~~~~~~~
mv: cannot stat 'perf/.libbpf.o.tmp': No such file or directory
tools/build/Makefile.build:96: recipe for target 'perf/libbpf.o' failed
Caused by commit
89b1698c93a9 ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/davem/net")
Some of the lines in tools/lib/bpf/btf.[ch] were duplicated (they existed
on both sides of the merge).
I have used the net-next tree from next-20180802 for today.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH nf-next v2 1/1] ipv6: defrag: drop non-last frags smaller than min mtu
From: Stephen Hemminger @ 2018-08-03 0:14 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev, Peter Oskolkov, Eric Dumazet
In-Reply-To: <20180803000539.17394-1-fw@strlen.de>
On Fri, 3 Aug 2018 02:05:39 +0200
Florian Westphal <fw@strlen.de> wrote:
> + if (-skb_network_offset(skb) + skb->len < IPV6_MIN_MTU &&
Is there a reason to write the arithmetic expression in this order?
Why not:
if (skb->len - skb_network_offset(skb) < IPV6_MIN_MTU &&
^ permalink raw reply
* Re: [PATCH v2 net-next 1/3] ip: discard IPv4 datagrams with overlapping segments.
From: Stephen Hemminger @ 2018-08-03 0:09 UTC (permalink / raw)
To: Peter Oskolkov; +Cc: David Miller, netdev, Eric Dumazet, Florian Westphal
In-Reply-To: <20180802233439.51643-2-posk@google.com>
On Thu, 2 Aug 2018 23:34:37 +0000
Peter Oskolkov <posk@google.com> wrote:
> This behavior is required in IPv6, and there is little need
> to tolerate overlapping fragments in IPv4. This change
> simplifies the code and eliminates potential DDoS attack vectors.
>
> Tested: ran ip_defrag selftest (not yet available uptream).
>
> Suggested-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Peter Oskolkov <posk@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Florian Westphal <fw@strlen.de>
There are a couple of relevant RFC's
RFC 1858 - Security Considerations for IP Fragment Filtering
RFC 2460 - Handling of Overlapping IPv6 Fragments
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply
* [PATCH nf-next v2 1/1] ipv6: defrag: drop non-last frags smaller than min mtu
From: Florian Westphal @ 2018-08-03 0:05 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal, Peter Oskolkov, Eric Dumazet
don't bother with pathological cases, they only waste cycles.
IPv6 requires a minimum MTU of 1280 so we should never see fragments
smaller than this (except last frag).
v2: drop IPv4 part, which added same check w. IPV4_MIN_MTU (68).
There were concerns that there could be even smaller frags
generated by intermediate nodes, e.g. on radio networks.
Cc: Peter Oskolkov <posk@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/ipv6/netfilter/nf_conntrack_reasm.c | 4 ++++
net/ipv6/reassembly.c | 4 ++++
2 files changed, 8 insertions(+)
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 0610bdab721c..c121d534d321 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -557,6 +557,10 @@ int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
hdr = ipv6_hdr(skb);
fhdr = (struct frag_hdr *)skb_transport_header(skb);
+ if (-skb_network_offset(skb) + skb->len < IPV6_MIN_MTU &&
+ fhdr->frag_off & htons(IP6_MF))
+ return -EINVAL;
+
skb_orphan(skb);
fq = fq_find(net, fhdr->identification, user, hdr,
skb->dev ? skb->dev->ifindex : 0);
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 6edd2ac8ae4b..ff00ada6128f 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -455,6 +455,10 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
return 1;
}
+ if (-skb_network_offset(skb) + skb->len < IPV6_MIN_MTU &&
+ fhdr->frag_off & htons(IP6_MF))
+ goto fail_hdr;
+
iif = skb->dev ? skb->dev->ifindex : 0;
fq = fq_find(net, fhdr->identification, hdr, iif);
if (fq) {
--
2.16.4
^ permalink raw reply related
* Re: [V9fs-developer] About the V9fs-developer list (Was: [PATCH] 9p: fix multiple
From: piaojun @ 2018-08-03 1:50 UTC (permalink / raw)
To: Dominique Martinet
Cc: Latchesar Ionkov, Eric Van Hensbergen, netdev, linux-kernel,
v9fs-developer, Ron Minnich
In-Reply-To: <20180802222835.GB18901@nautica>
On 2018/8/3 6:28, Dominique Martinet wrote:
> Dominique Martinet wrote on Sat, Jul 28, 2018:
>> piaojun wrote on Sat, Jul 28, 2018:
>>> Could we cancel the limitation of recipients' number?
>>
>> I think it's hard-coded by source-forge...
>> I'm not list admin for v9fs-developer but we've had problems with
>> another sf list on another project and the options are fairly limited
>> from what I can recall...
>
> I've got the list password from Eric (thanks!) and had a look.
>
> It's actually more open than I remembered, in particular I have found
> this maximum number of recipients setting and have lifted the limit.
>
>
> You probably noticed I've also accepted a bunch of messages just now,
> sorry for the spam if these were messages you had already seen, but
> let's look forward to a more active list ;)
>
Good news!
^ permalink raw reply
* Re: [PATCH net-next 1/3] ip: discard IPv4 datagrams with overlapping segments.
From: Stephen Hemminger @ 2018-08-02 23:56 UTC (permalink / raw)
To: Eric Dumazet
Cc: Peter Oskolkov, David Miller, netdev, Eric Dumazet,
Florian Westphal
In-Reply-To: <b8fcccc6-2946-b146-b136-3915c005fd26@gmail.com>
On Thu, 2 Aug 2018 16:33:55 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On 08/02/2018 03:54 PM, Stephen Hemminger wrote:
> > On Thu, 2 Aug 2018 22:45:58 +0000
> > Peter Oskolkov <posk@google.com> wrote:
> >
> >> diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h
> >> index e5ebc83827ab..da1a144f1a51 100644
> >> --- a/include/uapi/linux/snmp.h
> >> +++ b/include/uapi/linux/snmp.h
> >> @@ -40,6 +40,7 @@ enum
> >> IPSTATS_MIB_REASMREQDS, /* ReasmReqds */
> >> IPSTATS_MIB_REASMOKS, /* ReasmOKs */
> >> IPSTATS_MIB_REASMFAILS, /* ReasmFails */
> >> + IPSTATS_MIB_REASM_OVERLAPS, /* ReasmOverlaps */
> >> IPSTATS_MIB_FRAGOKS, /* FragOKs */
> >> IPSTATS_MIB_FRAGFAILS, /* FragFails */
> >> IPSTATS_MIB_FRAGCREATES, /* FragCreates */
> >
> > Inserting new entries in the middle of an enum means the numeric
> > values will change. Isn't this going to break userspace ABI?
> >
>
> I would argue the only exported thing from the kernel are the Key:Val in the /proc files.
>
> Not sure why these enum are uapi.
>
> Commit 46c2fa39877ed70415ee2b1acfb9129e956f6de4 added LINUX_MIB_TCPFASTOPENBLACKHOLE in the middle,
> and really nobody complained.
>
>
> Commit 0604475119de5f80dc051a5db055c6a2a75bd542 added LINUX_MIBSTCPMEMORYPRESSURESCHRONO
> in the middle as well.
>
> I am pretty sure we should maintain locality of these counters to lower number
> of dirtied cache lines, say under IP frag DDOS ;)
>
Agree with eric, go ahead and keep things local
^ 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