* Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.
From: Andrew Lunn @ 2019-09-27 14:56 UTC (permalink / raw)
To: Vincent Cheng
Cc: robh+dt@kernel.org, mark.rutland@arm.com,
richardcochran@gmail.com, devicetree@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190927141215.GA24424@renesas.com>
> >> +static void set_default_function_pointers(struct idtcm *idtcm)
> >> +{
> >> + idtcm->_idtcm_gettime = _idtcm_gettime;
> >> + idtcm->_idtcm_settime = _idtcm_settime;
> >> + idtcm->_idtcm_rdwr = idtcm_rdwr;
> >> + idtcm->_sync_pll_output = sync_pll_output;
> >> +}
> >
> >Why does this indirection? Are the SPI versions of the silicon?
>
> The indirection is to enable us to replace those functions in
> our unit tests with mocked functions.
Due to Spectra/meltdown etc, indirection is now expensive. But i guess
the I2C operations are a lot more expensive.
But in general, we try to keep the code KISS. Have you tried other
ways of doing this. Have your unit test framework implement
i2c_transfer()?
> I read somewhere that I should leave a week between sending a
> revised patch series. Is this a good rule to follow?
There are different 'timers'. One is how long to wait for review
comments, and reposting when you don't receiver any comments. netdev
for example is fast, a couple of days. Other subsystems, you need to
wait two weeks. Another 'timer' is how often to post new versions. In
general, never more than once per day. And the slower the subsystem is
for making reviews, the longer you should wait for additional review
comments.
What also plays a role is that the merge window is currently open. So
most subsystems won't accept patches at the moment. You need to wait
until it closes before submitting patches you expect to be good enough
to be accepted.
Andrew
^ permalink raw reply
* Re: genetlink: prevent memory leak in netlbl_unlabel_defconf
From: Paul Moore @ 2019-09-27 14:48 UTC (permalink / raw)
To: Markus Elfring
Cc: Navid Emamdoost, linux-security-module, netdev, Navid Emamdoost,
Kangjie Lu, linux-kernel, kernel-janitors, Stephen A McCamant,
David S. Miller
In-Reply-To: <c490685a-c7d6-5c95-5bf4-ed71f3c60cb6@web.de>
On Fri, Sep 27, 2019 at 9:15 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > > In netlbl_unlabel_defconf if netlbl_domhsh_add_default fails the
> > > allocated entry should be released.
> …
> > That said, netlbl_unlabel_defconf() *should* clean up here just on
> > principal if nothing else.
>
> How do you think about to add the tag “Fixes” then?
From what I've seen the "Fixes" tag is typically used by people who
are backporting patches, e.g. the -stable folks, to help decide what
they need to backport. As I mentioned in my previous email this
missing free doesn't actually manifest itself as a practical leak on
any of the existing kernels so there isn't a need to backport this
patch. For that reason I would probably skip the "Fixes" metadata
here, but I don't feel strongly enough about it to object if others
want it. FWIW, I play things very conservatively when talking about
backporting patches to stable kernels; if it doesn't fix a serious
user-visible bug it shouldn't be backported IMHO.
This patch is more of a conceptual fix than a practical fix. Not that
there is anything wrong with this patch, I just think it isn't as
critical as most people would think from reading "memory leak" in the
subject line. Yes, there is a memory leak, but the kernel panics soon
after so it's a bit moot. Further, even if the panic was somehow
skipped (?) the memory leak only happens once during boot; the failed
initialization is undoubtedly going to be far more damaging to the
system than a few lost bytes of memory.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* RE: [RFC 12/20] RDMA/irdma: Implement device supported verb APIs
From: Saleem, Shiraz @ 2019-09-27 14:28 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Kirsher, Jeffrey T, dledford@redhat.com, jgg@mellanox.com,
gregkh@linuxfoundation.org, Ismail, Mustafa,
netdev@vger.kernel.org, linux-rdma@vger.kernel.org
In-Reply-To: <20190927045029.GG14368@unreal>
> Subject: Re: [RFC 12/20] RDMA/irdma: Implement device supported verb APIs
>
> On Thu, Sep 26, 2019 at 07:49:52PM +0000, Saleem, Shiraz wrote:
> > > Subject: Re: [RFC 12/20] RDMA/irdma: Implement device supported verb
> > > APIs
> > >
> > > On Thu, Sep 26, 2019 at 09:45:11AM -0700, Jeff Kirsher wrote:
> > > > From: Mustafa Ismail <mustafa.ismail@intel.com>
> > > >
> > > > Implement device supported verb APIs. The supported APIs vary
> > > > based on the underlying transport the ibdev is registered as (i.e.
> > > > iWARP or RoCEv2).
> > > >
> > > > Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com>
> > > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> > > > ---
> > > > drivers/infiniband/hw/irdma/verbs.c | 4346 ++++++++++++++++++++++
> > > > drivers/infiniband/hw/irdma/verbs.h | 199 +
> > > > include/uapi/rdma/rdma_user_ioctl_cmds.h | 1 +
> > > > 3 files changed, 4546 insertions(+) create mode 100644
> > > > drivers/infiniband/hw/irdma/verbs.c
> > > > create mode 100644 drivers/infiniband/hw/irdma/verbs.h
> > > >
> > > > diff --git a/drivers/infiniband/hw/irdma/verbs.c
> > > > b/drivers/infiniband/hw/irdma/verbs.c
> > > > new file mode 100644
> > > > index 000000000000..025c21c722e2
> > > > --- /dev/null
> > > > +++ b/drivers/infiniband/hw/irdma/verbs.c
> > > > @@ -0,0 +1,4346 @@
> > > > +// SPDX-License-Identifier: GPL-2.0 or Linux-OpenIB
> > > > +/* Copyright (c) 2019, Intel Corporation. */
> > >
> > > <...>
> > >
> > > > +
> > > > + size = sqdepth * sizeof(struct irdma_sq_uk_wr_trk_info) +
> > > > + (rqdepth << 3);
> > > > + iwqp->kqp.wrid_mem = kzalloc(size, GFP_KERNEL);
> > > > + if (!iwqp->kqp.wrid_mem)
> > > > + return -ENOMEM;
> > > > +
> > > > + ukinfo->sq_wrtrk_array = (struct irdma_sq_uk_wr_trk_info *)
> > > > + iwqp->kqp.wrid_mem;
> > > > + if (!ukinfo->sq_wrtrk_array)
> > > > + return -ENOMEM;
> > >
> > > You are leaking resources here, forgot to do proper error unwinding.
> > >
> >
> > irdma_free_qp_rsrc() will free up that memory in case of an error.
>
> I'm talking about kqp.wrid_mem you allocated a couple of lines above and didn't
> free in case of sq_wrtrk_array allocation failed.
>
Yes, I am referring to kqp.wrid_mem as well
In case of err, all memory resources setup for
the QP is freed in the common utility irdma_free_qp_rsrc()
including the kqp.wrid_mem.
^ permalink raw reply
* RE: [RFC 20/20] RDMA/i40iw: Mark i40iw as deprecated
From: Saleem, Shiraz @ 2019-09-27 14:28 UTC (permalink / raw)
To: gregkh@linuxfoundation.org
Cc: Leon Romanovsky, Kirsher, Jeffrey T, dledford@redhat.com,
jgg@mellanox.com, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190926195517.GA1743170@kroah.com>
> Subject: Re: [RFC 20/20] RDMA/i40iw: Mark i40iw as deprecated
>
> On Thu, Sep 26, 2019 at 07:49:44PM +0000, Saleem, Shiraz wrote:
> > > Subject: Re: [RFC 20/20] RDMA/i40iw: Mark i40iw as deprecated
> > >
> > > On Thu, Sep 26, 2019 at 09:45:19AM -0700, Jeff Kirsher wrote:
> > > > From: Shiraz Saleem <shiraz.saleem@intel.com>
> > > >
> > > > Mark i40iw as deprecated/obsolete.
> > > >
> > > > irdma is the replacement driver that supports X722.
> > >
> > > Can you simply delete old one and add MODULE_ALIAS() in new driver?
> > >
> >
> > Yes, but we thought typically driver has to be deprecated for a few cycles
> before removing it.
>
> If you completely replace it with something that works the same, why keep the old
> one around at all?
Agree. Thanks!
>
> Unless you don't trust your new code? :)
>
We do :)
^ permalink raw reply
* RE: [RFC 15/20] RDMA/irdma: Add miscellaneous utility definitions
From: Saleem, Shiraz @ 2019-09-27 14:28 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Kirsher, Jeffrey T, dledford@redhat.com, jgg@mellanox.com,
gregkh@linuxfoundation.org, Ismail, Mustafa,
netdev@vger.kernel.org, linux-rdma@vger.kernel.org
In-Reply-To: <20190927044653.GF14368@unreal>
> Subject: Re: [RFC 15/20] RDMA/irdma: Add miscellaneous utility definitions
>
> On Thu, Sep 26, 2019 at 07:49:33PM +0000, Saleem, Shiraz wrote:
> > > Subject: Re: [RFC 15/20] RDMA/irdma: Add miscellaneous utility
> > > definitions
> > >
> > > On Thu, Sep 26, 2019 at 09:45:14AM -0700, Jeff Kirsher wrote:
> > > > From: Mustafa Ismail <mustafa.ismail@intel.com>
> > > >
> > > > Add miscellaneous utility functions and headers.
> > > >
> > > > Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com>
> > > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> > > > ---
> > > > drivers/infiniband/hw/irdma/osdep.h | 108 ++
> > > > drivers/infiniband/hw/irdma/protos.h | 96 ++
> > > > drivers/infiniband/hw/irdma/status.h | 70 +
> > > > drivers/infiniband/hw/irdma/utils.c | 2333
> > > > ++++++++++++++++++++++++++
> > > > 4 files changed, 2607 insertions(+) create mode 100644
> > > > drivers/infiniband/hw/irdma/osdep.h
> > > > create mode 100644 drivers/infiniband/hw/irdma/protos.h
> > > > create mode 100644 drivers/infiniband/hw/irdma/status.h
> > > > create mode 100644 drivers/infiniband/hw/irdma/utils.c
> > > >
> > > > diff --git a/drivers/infiniband/hw/irdma/osdep.h
> > > > b/drivers/infiniband/hw/irdma/osdep.h
> > > > new file mode 100644
> > > > index 000000000000..5885b6fa413d
> > > > --- /dev/null
> > > > +++ b/drivers/infiniband/hw/irdma/osdep.h
> > > > @@ -0,0 +1,108 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 or Linux-OpenIB */
> > > > +/* Copyright (c) 2019, Intel Corporation. */
> > > > +
> > > > +#ifndef IRDMA_OSDEP_H
> > > > +#define IRDMA_OSDEP_H
> > > > +
> > > > +#include <linux/version.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/vmalloc.h>
> > > > +#include <linux/string.h>
> > > > +#include <linux/bitops.h>
> > > > +#include <linux/pci.h>
> > > > +#include <net/tcp.h>
> > > > +#include <crypto/hash.h>
> > > > +/* get readq/writeq support for 32 bit kernels, use the low-first
> > > > +version */ #include <linux/io-64-nonatomic-lo-hi.h>
> > > > +
> > > > +#define MAKEMASK(m, s) ((m) << (s))
> > >
> > > It is a little bit over-macro.
> > >
> >
> > Why is this a problem?
> > We are not translating any basic kernel construct here.
>
> See BIT() definition.
>
OK. And?
^ permalink raw reply
* [GIT PULL] 9p updates for 5.4
From: Dominique Martinet @ 2019-09-27 14:27 UTC (permalink / raw)
To: Linus Torvalds; +Cc: v9fs-developer, linux-kernel, netdev
Hi Linus,
Some of the usual small fixes and cleanup.
I didn't get the target version wrong this time! :)
The following changes since commit 089cf7f6ecb266b6a4164919a2e69bd2f938374a:
Linux 5.3-rc7 (2019-09-02 09:57:40 -0700)
are available in the Git repository at:
https://github.com/martinetd/linux tags/9p-for-5.4
for you to fetch changes up to aafee43b72863f1f70aeaf1332d049916e8df239:
9p/vfs_super.c: Remove unused parameter data in v9fs_fill_super
(2019-09-03 11:10:13 +0000)
----------------------------------------------------------------
9p pull request for inclusion in 5.4
Small fixes all around:
- avoid overlayfs copy-up for PRIVATE mmaps
- KUMSAN uninitialized warning for transport error
- one syzbot memory leak fix in 9p cache
- internal API cleanup for v9fs_fill_super
----------------------------------------------------------------
Bharath Vedartham (2):
9p/cache.c: Fix memory leak in v9fs_cache_session_get_cookie
9p/vfs_super.c: Remove unused parameter data in v9fs_fill_super
Chengguang Xu (1):
9p: avoid attaching writeback_fid on mmap with type PRIVATE
Lu Shuaibing (1):
9p: Transport error uninitialized
fs/9p/cache.c | 2 ++
fs/9p/vfs_file.c | 3 +++
fs/9p/vfs_super.c | 4 ++--
net/9p/client.c | 1 +
4 files changed, 8 insertions(+), 2 deletions(-)
Cheers,
--
Dominique | Asmadeus
^ permalink raw reply
* Re: [PATCH] nfp: flower: prevent memory leak in nfp_flower_spawn_phy_reprs
From: Markus Elfring @ 2019-09-27 14:26 UTC (permalink / raw)
To: Navid Emamdoost, Jakub Kicinski, netdev, oss-drivers
Cc: Navid Emamdoost, Kangjie Lu, Stephen A McCamant, David S. Miller,
Frederik Lotter, John Hurley, Pieter Jansen van Vuuren,
Simon Horman, linux-kernel, kernel-janitors
In-Reply-To: <20190925182405.31287-1-navid.emamdoost@gmail.com>
> In nfp_flower_spawn_phy_reprs, in the for loop over eth_tbl if any of
> intermediate allocations or initializations fail memory is leaked.
> requiered releases are added.
I suggest to improve also this change description.
> @@ -542,6 +545,7 @@ nfp_flower_spawn_phy_reprs(struct nfp_app *app, struct nfp_flower_priv *priv)
> err = nfp_repr_init(app, repr,
> cmsg_port_id, port, priv->nn->dp.netdev);
> if (err) {
> + kfree(repr_priv);
> nfp_port_free(port);
> nfp_repr_free(repr);
> goto err_reprs_clean;
How do you think about to move common exception handling code
to the end of this function implementation by adding jump targets?
Regards,
Markus
^ permalink raw reply
* Re: pull-request: bpf 2019-09-27
From: David Miller @ 2019-09-27 14:26 UTC (permalink / raw)
To: daniel; +Cc: jakub.kicinski, ast, netdev, bpf
In-Reply-To: <20190927131508.24576-1-daniel@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Fri, 27 Sep 2019 15:15:08 +0200
> The following pull-request contains BPF updates for your *net* tree.
Pulled, thanks Daniel.
^ permalink raw reply
* Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.
From: Vincent Cheng @ 2019-09-27 14:12 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt@kernel.org, mark.rutland@arm.com,
richardcochran@gmail.com, devicetree@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190927122518.GA25474@lunn.ch>
On Fri, Sep 27, 2019 at 08:25:18AM EDT, Andrew Lunn wrote:
>> +static s32 idtcm_xfer(struct idtcm *idtcm,
>> + u8 regaddr,
>> + u8 *buf,
>> + u16 count,
>> + bool write)
>> +{
>> + struct i2c_client *client = idtcm->client;
>> + struct i2c_msg msg[2];
>> + s32 cnt;
>> +
>> + msg[0].addr = client->addr;
>> + msg[0].flags = 0;
>> + msg[0].len = 1;
>> + msg[0].buf = ®addr;
>> +
>> + msg[1].addr = client->addr;
>> + msg[1].flags = write ? 0 : I2C_M_RD;
>> + msg[1].len = count;
>> + msg[1].buf = buf;
>> +
>> + cnt = i2c_transfer(client->adapter, msg, 2);
>> +
>> + if (cnt < 0) {
>> + dev_err(&client->dev, "i2c_transfer returned %d\n", cnt);
>> + return cnt;
>> + } else if (cnt != 2) {
>> + dev_err(&client->dev,
>> + "i2c_transfer sent only %d of %d messages\n", cnt, 2);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static s32 idtcm_page_offset(struct idtcm *idtcm, u8 val)
>> +{
>> + u8 buf[4];
>> + s32 err;
>
>Hi Vincent
Hi Andrew,
Thank-you for looking at the patch.
>All your functions return s32, rather than the usual int. err is an
>s32. i2c_transfer() will return an int, which you then assign to an
>s32. I've no idea, but maybe the static code checkers like smatch
>will complain about this, especially on 64 bit systems? I suspect on
>64 bit machines, the compiler will be generating worse code, masking
>registers? Maybe use int, not s32?
Oops. You are correct, I messed up when trying to standardize
on linux types.h. I will go through the code to ensure int is used
for error codes and return values.
>> + case OUTPUT_MASK_PLL2_ADDR + 1:
>> + SET_U16_MSB(idtcm->channel[2].output_mask, val);
>> + break;
>> + case OUTPUT_MASK_PLL3_ADDR:
>> + SET_U16_LSB(idtcm->channel[3].output_mask, val);
>> + break;
>> + case OUTPUT_MASK_PLL3_ADDR + 1:
>> + SET_U16_MSB(idtcm->channel[3].output_mask, val);
>> + break;
>> + default:
>> + err = -1;
>
>EINVAL?
Yes, will replace with -EINVAL. Thanks.
>> +static void set_default_function_pointers(struct idtcm *idtcm)
>> +{
>> + idtcm->_idtcm_gettime = _idtcm_gettime;
>> + idtcm->_idtcm_settime = _idtcm_settime;
>> + idtcm->_idtcm_rdwr = idtcm_rdwr;
>> + idtcm->_sync_pll_output = sync_pll_output;
>> +}
>
>Why does this indirection? Are the SPI versions of the silicon?
The indirection is to enable us to replace those functions in
our unit tests with mocked functions.
I read somewhere that I should leave a week between sending a
revised patch series. Is this a good rule to follow?
Regards,
Vincent
^ permalink raw reply
* Re: [PATCH] nfp: flower: fix memory leak in nfp_flower_spawn_vnic_reprs
From: Markus Elfring @ 2019-09-27 14:06 UTC (permalink / raw)
To: Navid Emamdoost, Jakub Kicinski, netdev, oss-drivers
Cc: Navid Emamdoost, Kangjie Lu, Stephen A McCamant, David S. Miller,
Frederik Lotter, John Hurley, Pieter Jansen van Vuuren,
Simon Horman, linux-kernel, kernel-janitors
In-Reply-To: <20190925190512.3404-1-navid.emamdoost@gmail.com>
> @@ -433,6 +435,7 @@ nfp_flower_spawn_vnic_reprs(struct nfp_app *app,
> err = nfp_repr_init(app, repr,
> port_id, port, priv->nn->dp.netdev);
> if (err) {
> + kfree(repr_priv);
> nfp_port_free(port);
> nfp_repr_free(repr);
> goto err_reprs_clean;
How do you think about to move common exception handling code
to the end of this function implementation by using another jump target?
Regards,
Markus
^ permalink raw reply
* Re: net/ncsi: prevent memory leak in ncsi_rsp_handler_gc
From: Markus Elfring @ 2019-09-27 13:40 UTC (permalink / raw)
To: Navid Emamdoost, netdev
Cc: Navid Emamdoost, Kangjie Lu, Stephen A McCamant, Al Viro,
David S. Miller, Samuel Mendoza-Jonas, linux-kernel,
kernel-janitors
In-Reply-To: <20190927031501.GF22969@cs-dulles.cs.umn.edu>
> > > In ncsi_rsp_handler_gc if allocation for nc->vlan_filter.vids fails the
> > > allocated memory for nc->mac_filter.addrs should be released.
…
> The problem is that just by traversing the code using tools
> like ctags or elixir I couldn't find any caller to ncsi_rsp_handler_gc
> that handles such errnos.
Would you like to collaborate with higher level source code analysis tools?
How do you think about to add the tag “Fixes” here?
Regards,
Markus
^ permalink raw reply
* Re: [PATCH net] devlink: Fix error handling in param and info_get dumpit cb
From: Vasundhara Volam @ 2019-09-27 13:37 UTC (permalink / raw)
To: Andrew Lunn; +Cc: David Miller, Netdev, Jiri Pirko, Michael Chan
In-Reply-To: <20190927123129.GB25474@lunn.ch>
On Fri, Sep 27, 2019 at 6:01 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Sep 27, 2019 at 10:28:36AM +0530, Vasundhara Volam wrote:
> > On Thu, Sep 26, 2019 at 5:57 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Thu, Sep 26, 2019 at 03:05:54PM +0530, Vasundhara Volam wrote:
> > > > If any of the param or info_get op returns error, dumpit cb is
> > > > skipping to dump remaining params or info_get ops for all the
> > > > drivers.
> > > >
> > > > Instead skip only for the param/info_get op which returned error
> > > > and continue to dump remaining information, except if the return
> > > > code is EMSGSIZE.
> > >
> > > Hi Vasundhara
> > >
> > > How do we get to see something did fail? If it failed, it failed for a
> > > reason, and we want to know.
> > >
> > > What is your real use case here? What is failing, and why are you
> > > O.K. to skip this failure?
> > >
> > > Andrew
> > Hi Andrew,
> >
> > Thank you for looking into the patch.
> >
> > If any of the devlink parameter is returning error like EINVAL, then
> > current code is not displaying any further parameters for all the other
> > devices as well.
> >
> > In bnxt_en driver case, some of the parameters are not supported in
> > certain configurations like if the parameter is not part of the
> > NVM configuration, driver returns EINVAL error to the stack. And devlink is
> > skipping to display all the remaining parameters for that device and others
> > as well.
> >
> > I am trying to fix to skip only the error parameter and display the remaining
> > parameters.
>
> Hi Vasundhara
>
> Thanks for explaining your use case. It sounds sensible. But i would
> narrow this down.
>
> Make the driver return EOPNOTSUP, not EINVAL. And then in dump, only
> skip EOPNOTSUP. Any other errors cause the error to be returned, so we
> get to see them.
Thank you Andrew, I will modify the patch and resubmit.
>
> Andrew
^ permalink raw reply
* Re: [PATCH] vsock/virtio: add support for MSG_PEEK
From: Eric Dumazet @ 2019-09-27 13:37 UTC (permalink / raw)
To: Stefano Garzarella, Eric Dumazet, Matias Ezequiel Vara Larsen
Cc: stefanha, davem, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20190927085513.tdiofiisrpyehfe5@steredhat.homenet.telecomitalia.it>
On 9/27/19 1:55 AM, Stefano Garzarella wrote:
> Good catch!
>
> Maybe we can solve in this way:
>
> list_for_each_entry(pkt, &vvs->rx_queue, list) {
> size_t off = pkt->off;
>
> if (total == len)
> break;
>
> while (total < len && off < pkt->len) {
> /* using 'off' instead of 'pkt->off' */
> ...
>
> total += bytes;
> off += bytes;
> }
> }
>
> What do you think?
>
Maybe, but I need to see a complete patch, evil is in the details :)
^ permalink raw reply
* Re: [PATCH] vhost: introduce mdev based hardware backend
From: Michael S. Tsirkin @ 2019-09-27 13:23 UTC (permalink / raw)
To: Jason Wang
Cc: Tiwei Bie, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang,
lingshan.zhu
In-Reply-To: <b6f6ffb2-0b16-5041-be2e-94b805c6a4c9@redhat.com>
On Fri, Sep 27, 2019 at 09:17:56PM +0800, Jason Wang wrote:
>
> On 2019/9/27 下午8:46, Michael S. Tsirkin wrote:
> > On Fri, Sep 27, 2019 at 08:17:47PM +0800, Jason Wang wrote:
> > > On 2019/9/27 下午5:41, Michael S. Tsirkin wrote:
> > > > On Fri, Sep 27, 2019 at 11:27:12AM +0800, Jason Wang wrote:
> > > > > On 2019/9/26 下午9:14, Tiwei Bie wrote:
> > > > > > On Thu, Sep 26, 2019 at 04:35:18AM -0400, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Sep 26, 2019 at 12:54:27PM +0800, Tiwei Bie wrote:
> > > > > > [...]
> > > > > > > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > > > > > > index 40d028eed645..5afbc2f08fa3 100644
> > > > > > > > --- a/include/uapi/linux/vhost.h
> > > > > > > > +++ b/include/uapi/linux/vhost.h
> > > > > > > > @@ -116,4 +116,12 @@
> > > > > > > > #define VHOST_VSOCK_SET_GUEST_CID _IOW(VHOST_VIRTIO, 0x60, __u64)
> > > > > > > > #define VHOST_VSOCK_SET_RUNNING _IOW(VHOST_VIRTIO, 0x61, int)
> > > > > > > > +/* VHOST_MDEV specific defines */
> > > > > > > > +
> > > > > > > > +#define VHOST_MDEV_SET_STATE _IOW(VHOST_VIRTIO, 0x70, __u64)
> > > > > > > > +
> > > > > > > > +#define VHOST_MDEV_S_STOPPED 0
> > > > > > > > +#define VHOST_MDEV_S_RUNNING 1
> > > > > > > > +#define VHOST_MDEV_S_MAX 2
> > > > > > > > +
> > > > > > > > #endif
> > > > > > > So assuming we have an underlying device that behaves like virtio:
> > > > > > I think they are really good questions/suggestions. Thanks!
> > > > > >
> > > > > > > 1. Should we use SET_STATUS maybe?
> > > > > > I like this idea. I will give it a try.
> > > > > >
> > > > > > > 2. Do we want a reset ioctl?
> > > > > > I think it is helpful. If we use SET_STATUS, maybe we
> > > > > > can use it to support the reset.
> > > > > >
> > > > > > > 3. Do we want ability to enable rings individually?
> > > > > > I will make it possible at least in the vhost layer.
> > > > > Note the API support e.g set_vq_ready().
> > > > virtio spec calls this "enabled" so let's stick to that.
> > >
> > > Ok.
> > >
> > >
> > > > > > > 4. Does device need to limit max ring size?
> > > > > > > 5. Does device need to limit max number of queues?
> > > > > > I think so. It's helpful to have ioctls to report the max
> > > > > > ring size and max number of queues.
> > > > > An issue is the max number of queues is done through a device specific way,
> > > > > usually device configuration space. This is supported by the transport API,
> > > > > but how to expose it to userspace may need more thought.
> > > > >
> > > > > Thanks
> > > > an ioctl for device config? But for v1 I'd be quite happy to just have
> > > > a minimal working device with 2 queues.
> > >
> > > I'm fully agree, and it will work as long as VIRTIO_NET_F_MQ and
> > > VIRTIO_NET_F_CTRL_VQ is not advertised by the mdev device.
> > >
> > > Thanks
> > Hmm this means we need to validate the features bits,
> > not just pass them through to the hardware.
> > Problem is, how do we add more feature bits later,
> > without testing all hardware?
> > I guess this means the device specific driver must do it.
> >
>
> That looks not good, maybe a virtio device id based features blacklist in
> vhost-mdev. Then MQ and CTRL_VQ could be filtered out by vhost-mdev.
>
> Thanks
Two implementations of e.g. virtio net can have different
features whitelisted. So I think there's no way but let
the driver do it. We should probably provide a standard place
in the ops for driver to supply the whitelist, to make sure
drivers don't forget.
>
> > > > > > Thanks!
^ permalink raw reply
* Re: [PATCH net] tcp: better handle TCP_USER_TIMEOUT in SYN_SENT state
From: Eric Dumazet @ 2019-09-27 13:18 UTC (permalink / raw)
To: Marek Majkowski
Cc: David Miller, Jonathan Maxwell, Eric Dumazet, netdev,
Yuchung Cheng, kernel-team
In-Reply-To: <20190927124017.26996-1-marek@cloudflare.com>
On Fri, Sep 27, 2019 at 5:40 AM Marek Majkowski <marek@cloudflare.com> wrote:
>
> On 9/27/19 10:25 AM, Jonathan Maxwell wrote:
> > Acked-by: Jon Maxwell <jmaxwell37@gmail.com>
> >
> > Thanks for fixing that Eric.
> >
>
> The patch seems to do the job.
>
> Tested-by: Marek Majkowski <marek@cloudflare.com>
>
> Here's a selftest:
>
Here is the packetdrill test :
`../common/defaults.sh`
0 socket(..., SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP) = 4
+0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [8000], 4) = 0
+0 sendto(4, ..., 1000, MSG_FASTOPEN, ..., ...) = -1
+0 > S 0:0(0) <mss 1460,sackOK,TS val 100 ecr 0,nop,wscale 8>
+1 > S 0:0(0) <mss 1460,sackOK,TS val 1100 ecr 0,nop,wscale 8>
+2 > S 0:0(0) <mss 1460,sackOK,TS val 3100 ecr 0,nop,wscale 8>
+4 > S 0:0(0) <mss 1460,sackOK,TS val 7100 ecr 0,nop,wscale 8>
+10 write(4, ..., 1000) = -1 ETIMEDOUT (Connection Timed Out)
+0 %{ assert tcpi_state == TCP_CLOSE }%
+0 < S. 1234:1234(0) ack 1 win 5840 <mss 1460,sackOK,TS val 987134
ecr 7100,nop,wscale 8>
+0 > R 1:1(0)
> ---8<---
> From: Marek Majkowski <marek@cloudflare.com>
> Date: Fri, 27 Sep 2019 13:37:52 +0200
> Subject: [PATCH] selftests/net: TCP_USER_TIMEOUT in SYN-SENT state
>
> Test the TCP_USER_TIMEOUT behavior, overriding TCP_SYNCNT
> when socket is in SYN-SENT state.
>
> Signed-off-by: Marek Majkowski <marek@cloudflare.com>
> ---
> tools/testing/selftests/net/.gitignore | 1 +
> tools/testing/selftests/net/Makefile | 3 +-
> .../selftests/net/tcp_user_timeout_syn_sent.c | 322 ++++++++++++++++++
> .../net/tcp_user_timeout_syn_sent.sh | 4 +
> 4 files changed, 329 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/net/tcp_user_timeout_syn_sent.c
> create mode 100755 tools/testing/selftests/net/tcp_user_timeout_syn_sent.sh
>
> diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
> index c7cced739c34..bc6a2b7199b6 100644
> --- a/tools/testing/selftests/net/.gitignore
> +++ b/tools/testing/selftests/net/.gitignore
> @@ -21,3 +21,4 @@ ipv6_flowlabel
> ipv6_flowlabel_mgr
> so_txtime
> tcp_fastopen_backup_key
> +tcp_user_timeout_syn_sent
> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> index 0bd6b23c97ef..065a171b8834 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -11,13 +11,14 @@ TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh
> TEST_PROGS += udpgro_bench.sh udpgro.sh test_vxlan_under_vrf.sh reuseport_addr_any.sh
> TEST_PROGS += test_vxlan_fdb_changelink.sh so_txtime.sh ipv6_flowlabel.sh
> TEST_PROGS += tcp_fastopen_backup_key.sh fcnal-test.sh l2tp.sh
> +TEST_PROGS += tcp_user_timeout_syn_sent.sh
> TEST_PROGS_EXTENDED := in_netns.sh
> TEST_GEN_FILES = socket nettest
> TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any
> TEST_GEN_FILES += tcp_mmap tcp_inq psock_snd txring_overwrite
> TEST_GEN_FILES += udpgso udpgso_bench_tx udpgso_bench_rx ip_defrag
> TEST_GEN_FILES += so_txtime ipv6_flowlabel ipv6_flowlabel_mgr
> -TEST_GEN_FILES += tcp_fastopen_backup_key
> +TEST_GEN_FILES += tcp_fastopen_backup_key tcp_user_timeout_syn_sent
> TEST_GEN_PROGS = reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
> TEST_GEN_PROGS += reuseport_dualstack reuseaddr_conflict tls
>
> diff --git a/tools/testing/selftests/net/tcp_user_timeout_syn_sent.c b/tools/testing/selftests/net/tcp_user_timeout_syn_sent.c
> new file mode 100644
> index 000000000000..1c9ec582359a
> --- /dev/null
> +++ b/tools/testing/selftests/net/tcp_user_timeout_syn_sent.c
> @@ -0,0 +1,322 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Testing if TCP_USER_TIMEOUT on SYN-SENT state overides TCP_SYNCNT.
> + *
> + * Historically, TCP_USER_TIMEOUT made only sense on synchronized TCP
> + * states, like ESTABLISHED. There was a bug on SYN-SENT state: with
> + * TCP_USER_TIMEOUT set, the connect() would ETIMEDOUT after given
> + * time, but near the end of the timer would flood SYN packets to
> + * fulfill the TCP_SYNCNT counter. For example for 2000ms user
> + * timeout and default TCP_SYNCNT=6, the tcpdump would look like:
> + *
> + * 00:00.000000 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
> + * 00:01.029452 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
> + * 00:02.021354 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
> + * 00:02.033419 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
> + * 00:02.041633 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
> + * 00:02.049263 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
> + * 00:02.057264 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
> + *
> + * Notice, 5 out of 6 retransmissions are aligned to 2s. We fixed
> + * that, and this code tests for the regression. We do this by
> + * actively dropping SYN packets on listen socket with ebpf
> + * SOCKET_FILTER, and counting how many packets did we drop.
> + *
> + * See: https://blog.cloudflare.com/when-tcp-sockets-refuse-to-die/
> + */
> +#include <arpa/inet.h>
> +#include <errno.h>
> +#include <error.h>
> +#include <linux/bpf.h>
> +#include <linux/tcp.h>
> +#include <linux/unistd.h>
> +#include <netinet/in.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/socket.h>
> +#include <unistd.h>
> +
> +int bpf_create_map(enum bpf_map_type map_type, int key_size, int value_size,
> + int max_entries, uint32_t map_flags)
> +{
> + union bpf_attr attr = {};
> +
> + attr.map_type = map_type;
> + attr.key_size = key_size;
> + attr.value_size = value_size;
> + attr.max_entries = max_entries;
> + attr.map_flags = map_flags;
> + return syscall(__NR_bpf, BPF_MAP_CREATE, &attr, sizeof(attr));
> +}
> +
> +int bpf_load_program(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
> + size_t insns_cnt, const char *license,
> + uint32_t kern_version)
> +{
> + union bpf_attr attr = {};
> +
> + attr.prog_type = prog_type;
> + attr.insns = (long)insns;
> + attr.insn_cnt = insns_cnt;
> + attr.license = (long)license;
> + attr.log_buf = (long)NULL;
> + attr.log_size = 0;
> + attr.log_level = 0;
> + attr.kern_version = kern_version;
> +
> + int fd = syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr));
> + return fd;
> +}
> +
> +int bpf_map_update_elem(int fd, const void *key, const void *value,
> + uint64_t flags)
> +{
> + union bpf_attr attr = {};
> +
> + attr.map_fd = fd;
> + attr.key = (long)key;
> + attr.value = (long)value;
> + attr.flags = flags;
> +
> + return syscall(__NR_bpf, BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr));
> +}
> +
> +int bpf_map_lookup_elem(int fd, const void *key, void *value)
> +{
> + union bpf_attr attr = {};
> +
> + attr.map_fd = fd;
> + attr.key = (long)key;
> + attr.value = (long)value;
> +
> + return syscall(__NR_bpf, BPF_MAP_LOOKUP_ELEM, &attr, sizeof(attr));
> +}
> +
> +/*
> +struct bpf_map_def SEC("maps") stats_map = {
> + .type = BPF_MAP_TYPE_ARRAY,
> + .key_size = sizeof(uint32_t),
> + .value_size = sizeof(uint64_t),
> + .max_entries = 2,
> +};
> +
> +SEC("socket_filter")
> +int _socket_filter(struct __sk_buff *skb)
> +{
> + (void)skb;
> +
> + uint32_t no = 0;
> + uint64_t *value = bpf_map_lookup_elem(&stats_map, &no);
> + if (value) {
> + __sync_fetch_and_add(value, 1);
> + }
> + return 0; // DROP inbound SYN packets
> +}
> + */
> +
> +size_t bpf_insn_socket_filter_cnt = 12;
> +struct bpf_insn bpf_insn_socket_filter[] = {
> + {
> + .code = 0xb7,
> + .dst_reg = BPF_REG_1,
> + .src_reg = BPF_REG_0,
> + .off = 0,
> + .imm = 0 /**/
> + },
> + {
> + .code = 0x63,
> + .dst_reg = BPF_REG_10,
> + .src_reg = BPF_REG_1,
> + .off = -4,
> + .imm = 0 /**/
> + },
> + {
> + .code = 0xbf,
> + .dst_reg = BPF_REG_2,
> + .src_reg = BPF_REG_10,
> + .off = 0,
> + .imm = 0 /**/
> + },
> + {
> + .code = 0x7,
> + .dst_reg = BPF_REG_2,
> + .src_reg = BPF_REG_0,
> + .off = 0,
> + .imm = -4 /**/
> + },
> + {
> + .code = 0x18,
> + .dst_reg = BPF_REG_1,
> + .src_reg = BPF_REG_0,
> + .off = 0,
> + .imm = 0 /* relocation for stats_map */
> + },
> + {
> + .code = 0x0,
> + .dst_reg = BPF_REG_0,
> + .src_reg = BPF_REG_0,
> + .off = 0,
> + .imm = 0 /**/
> + },
> + {
> + .code = 0x85,
> + .dst_reg = BPF_REG_0,
> + .src_reg = BPF_REG_0,
> + .off = 0,
> + .imm = 1 /**/
> + },
> + {
> + .code = 0x15,
> + .dst_reg = BPF_REG_0,
> + .src_reg = BPF_REG_0,
> + .off = 2,
> + .imm = 0 /**/
> + },
> + {
> + .code = 0xb7,
> + .dst_reg = BPF_REG_1,
> + .src_reg = BPF_REG_0,
> + .off = 0,
> + .imm = 1 /**/
> + },
> + {
> + .code = 0xdb,
> + .dst_reg = BPF_REG_0,
> + .src_reg = BPF_REG_1,
> + .off = 0,
> + .imm = 0 /**/
> + },
> + {
> + .code = 0xb7,
> + .dst_reg = BPF_REG_0,
> + .src_reg = BPF_REG_0,
> + .off = 0,
> + .imm = 0 /**/
> + },
> + {
> + .code = 0x95,
> + .dst_reg = BPF_REG_0,
> + .src_reg = BPF_REG_0,
> + .off = 0,
> + .imm = 0 /**/
> + }
> +};
> +
> +void socket_filter_fill_stats_map(int fd)
> +{
> + bpf_insn_socket_filter[4].src_reg = BPF_REG_1;
> + bpf_insn_socket_filter[4].imm = fd;
> +}
> +
> +static int net_setup_ebpf(int sd)
> +{
> + int stats_map, bpf, r;
> +
> + stats_map = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(uint32_t),
> + sizeof(uint64_t), 2, 0);
> + if (stats_map < 0)
> + error(1, errno, "bpf");
> +
> + socket_filter_fill_stats_map(stats_map);
> +
> + bpf = bpf_load_program(BPF_PROG_TYPE_SOCKET_FILTER,
> + bpf_insn_socket_filter,
> + bpf_insn_socket_filter_cnt, "Dual BSD/GPL", 0);
> + if (bpf < 0)
> + error(1, errno, "bpf");
> +
> + r = setsockopt(sd, SOL_SOCKET, SO_ATTACH_BPF, &bpf, sizeof(bpf));
> + if (r < 0)
> + error(1, errno, "setsockopt(SO_ATTACH_FILTER)");
> +
> + return stats_map;
> +}
> +
> +static int setup_server(struct sockaddr_in *addr)
> +{
> + int sd, r;
> + socklen_t addr_sz;
> +
> + sd = socket(AF_INET, SOCK_STREAM, 0);
> + if (sd < 0)
> + error(1, errno, "socket()");
> +
> + r = bind(sd, (struct sockaddr *)addr, sizeof(*addr));
> + if (r != 0)
> + error(1, errno, "bind()");
> +
> + r = listen(sd, 16);
> + if (r != 0)
> + error(1, errno, "listen()");
> +
> + addr_sz = sizeof(*addr);
> + r = getsockname(sd, (struct sockaddr *)addr, &addr_sz);
> + if (r != 0)
> + error(1, errno, "getsockname()");
> +
> + return sd;
> +}
> +
> +int main(void)
> +{
> + struct sockaddr_in addr = {
> + .sin_family = AF_INET,
> + .sin_addr = { inet_addr("127.0.0.1") },
> + };
> +
> + int sd = setup_server(&addr);
> + int stats_map = net_setup_ebpf(sd);
> + struct {
> + int user_timeout;
> + int expected_counter;
> + } tests[] = {
> + { 200, 2 }, // TCP_USER_TIMEOUT kicks in on first retranmission
> + { 1500, 2 },
> + { 3500, 3 },
> + { -1, -1 },
> + };
> +
> + int failed = 0, i;
> +
> + for (i = 0; tests[i].user_timeout >= 0; i++) {
> + int r, cd, v;
> + uint32_t k = 0;
> + uint64_t counter = 0;
> +
> + r = bpf_map_update_elem(stats_map, &k, &counter, 0);
> +
> + cd = socket(AF_INET, SOCK_STREAM, 0);
> + if (cd < 0)
> + error(1, errno, "socket()");
> +
> + v = tests[i].user_timeout;
> + r = setsockopt(cd, IPPROTO_TCP, TCP_USER_TIMEOUT, &v,
> + sizeof(v));
> + if (r != 0)
> + error(1, errno, "setsockopt()");
> +
> + r = connect(cd, (struct sockaddr *)&addr, sizeof(addr));
> + if (r != -1 && errno != ETIMEDOUT)
> + error(1, errno, "connect()");
> +
> + r = bpf_map_lookup_elem(stats_map, &k, &counter);
> + if (r != 0)
> + error(1, errno, "bpf_map_lookup_elem()");
> +
> + if ((int)counter != tests[i].expected_counter) {
> + failed += 1;
> + printf("[!] Expecting %d SYN packets on "
> + "TCP_USER_TIMEOUT=%d, got %d\n",
> + tests[i].expected_counter, tests[i].user_timeout,
> + (int)counter);
> + }
> + close(cd);
> + }
> + close(sd);
> + close(stats_map);
> + if (failed == 0)
> + fprintf(stderr, "PASSED\n");
> + return failed;
> +}
> diff --git a/tools/testing/selftests/net/tcp_user_timeout_syn_sent.sh b/tools/testing/selftests/net/tcp_user_timeout_syn_sent.sh
> new file mode 100755
> index 000000000000..26765f3a92c6
> --- /dev/null
> +++ b/tools/testing/selftests/net/tcp_user_timeout_syn_sent.sh
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +
> +./in_netns.sh ./tcp_user_timeout_syn_sent
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH] vhost: introduce mdev based hardware backend
From: Jason Wang @ 2019-09-27 13:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Tiwei Bie, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang,
lingshan.zhu
In-Reply-To: <20190927084408-mutt-send-email-mst@kernel.org>
On 2019/9/27 下午8:46, Michael S. Tsirkin wrote:
> On Fri, Sep 27, 2019 at 08:17:47PM +0800, Jason Wang wrote:
>> On 2019/9/27 下午5:41, Michael S. Tsirkin wrote:
>>> On Fri, Sep 27, 2019 at 11:27:12AM +0800, Jason Wang wrote:
>>>> On 2019/9/26 下午9:14, Tiwei Bie wrote:
>>>>> On Thu, Sep 26, 2019 at 04:35:18AM -0400, Michael S. Tsirkin wrote:
>>>>>> On Thu, Sep 26, 2019 at 12:54:27PM +0800, Tiwei Bie wrote:
>>>>> [...]
>>>>>>> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
>>>>>>> index 40d028eed645..5afbc2f08fa3 100644
>>>>>>> --- a/include/uapi/linux/vhost.h
>>>>>>> +++ b/include/uapi/linux/vhost.h
>>>>>>> @@ -116,4 +116,12 @@
>>>>>>> #define VHOST_VSOCK_SET_GUEST_CID _IOW(VHOST_VIRTIO, 0x60, __u64)
>>>>>>> #define VHOST_VSOCK_SET_RUNNING _IOW(VHOST_VIRTIO, 0x61, int)
>>>>>>> +/* VHOST_MDEV specific defines */
>>>>>>> +
>>>>>>> +#define VHOST_MDEV_SET_STATE _IOW(VHOST_VIRTIO, 0x70, __u64)
>>>>>>> +
>>>>>>> +#define VHOST_MDEV_S_STOPPED 0
>>>>>>> +#define VHOST_MDEV_S_RUNNING 1
>>>>>>> +#define VHOST_MDEV_S_MAX 2
>>>>>>> +
>>>>>>> #endif
>>>>>> So assuming we have an underlying device that behaves like virtio:
>>>>> I think they are really good questions/suggestions. Thanks!
>>>>>
>>>>>> 1. Should we use SET_STATUS maybe?
>>>>> I like this idea. I will give it a try.
>>>>>
>>>>>> 2. Do we want a reset ioctl?
>>>>> I think it is helpful. If we use SET_STATUS, maybe we
>>>>> can use it to support the reset.
>>>>>
>>>>>> 3. Do we want ability to enable rings individually?
>>>>> I will make it possible at least in the vhost layer.
>>>> Note the API support e.g set_vq_ready().
>>> virtio spec calls this "enabled" so let's stick to that.
>>
>> Ok.
>>
>>
>>>>>> 4. Does device need to limit max ring size?
>>>>>> 5. Does device need to limit max number of queues?
>>>>> I think so. It's helpful to have ioctls to report the max
>>>>> ring size and max number of queues.
>>>> An issue is the max number of queues is done through a device specific way,
>>>> usually device configuration space. This is supported by the transport API,
>>>> but how to expose it to userspace may need more thought.
>>>>
>>>> Thanks
>>> an ioctl for device config? But for v1 I'd be quite happy to just have
>>> a minimal working device with 2 queues.
>>
>> I'm fully agree, and it will work as long as VIRTIO_NET_F_MQ and
>> VIRTIO_NET_F_CTRL_VQ is not advertised by the mdev device.
>>
>> Thanks
> Hmm this means we need to validate the features bits,
> not just pass them through to the hardware.
> Problem is, how do we add more feature bits later,
> without testing all hardware?
> I guess this means the device specific driver must do it.
>
That looks not good, maybe a virtio device id based features blacklist
in vhost-mdev. Then MQ and CTRL_VQ could be filtered out by vhost-mdev.
Thanks
>>>>> Thanks!
^ permalink raw reply
* Re: genetlink: prevent memory leak in netlbl_unlabel_defconf
From: Markus Elfring @ 2019-09-27 13:15 UTC (permalink / raw)
To: Paul Moore, Navid Emamdoost, linux-security-module, netdev
Cc: Navid Emamdoost, Kangjie Lu, linux-kernel, kernel-janitors,
Stephen A McCamant, David S. Miller
In-Reply-To: <CAHC9VhR+4pZObDz7kG+rxnox2ph4z_wpZdyOL=WmdnRvdQNH9A@mail.gmail.com>
> > In netlbl_unlabel_defconf if netlbl_domhsh_add_default fails the
> > allocated entry should be released.
…
> That said, netlbl_unlabel_defconf() *should* clean up here just on
> principal if nothing else.
How do you think about to add the tag “Fixes” then?
Regards,
Markus
^ permalink raw reply
* pull-request: bpf 2019-09-27
From: Daniel Borkmann @ 2019-09-27 13:15 UTC (permalink / raw)
To: davem; +Cc: jakub.kicinski, daniel, ast, netdev, bpf
Hi David,
The following pull-request contains BPF updates for your *net* tree.
The main changes are:
1) Fix libbpf's BTF dumper to not skip anonymous enum definitions, from Andrii.
2) Fix BTF verifier issues when handling the BTF of vmlinux, from Alexei.
3) Fix nested calls into bpf_event_output() from TCP sockops BPF
programs, from Allan.
4) Fix NULL pointer dereference in AF_XDP's xsk map creation when
allocation fails, from Jonathan.
5) Remove unneeded 64 byte alignment requirement of the AF_XDP UMEM
headroom, from Bjorn.
6) Remove unused XDP_OPTIONS getsockopt() call which results in an error
on older kernels, from Toke.
7) Fix a client/server race in tcp_rtt BPF kselftest case, from Stanislav.
8) Fix indentation issue in BTF's btf_enum_check_kflag_member(), from Colin.
Please consider pulling these changes from:
git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git
Thanks a lot!
----------------------------------------------------------------
The following changes since commit 280ceaed79f18db930c0cc8bb21f6493490bf29c:
usbnet: sanity checking of packet sizes and device mtu (2019-09-19 13:27:11 +0200)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git
for you to fetch changes up to 768fb61fcc13b2acaca758275d54c09a65e2968b:
bpf: Fix bpf_event_output re-entry issue (2019-09-27 11:24:29 +0200)
----------------------------------------------------------------
Alexei Starovoitov (2):
bpf: fix BTF verification of enums
bpf: fix BTF limits
Allan Zhang (1):
bpf: Fix bpf_event_output re-entry issue
Andrii Nakryiko (4):
libbpf: fix false uninitialized variable warning
selftests/bpf: delete unused variables in test_sysctl
selftests/bpf: adjust strobemeta loop to satisfy latest clang
libbpf: Teach btf_dumper to emit stand-alone anonymous enum definitions
Björn Töpel (1):
xsk: relax UMEM headroom alignment
Colin Ian King (1):
bpf: Clean up indentation issue in BTF kflag processing
Jonathan Lemon (1):
bpf/xskmap: Return ERR_PTR for failure case instead of NULL.
Stanislav Fomichev (1):
selftests/bpf: test_progs: fix client/server race in tcp_rtt
Toke Høiland-Jørgensen (1):
libbpf: Remove getsockopt() check for XDP_OPTIONS
include/uapi/linux/btf.h | 4 +-
kernel/bpf/btf.c | 7 +-
kernel/bpf/xskmap.c | 2 +-
kernel/trace/bpf_trace.c | 26 +++++--
net/xdp/xdp_umem.c | 2 -
tools/lib/bpf/btf_dump.c | 94 ++++++++++++++++++++++--
tools/lib/bpf/xsk.c | 11 ---
tools/testing/selftests/bpf/prog_tests/tcp_rtt.c | 21 +++++-
tools/testing/selftests/bpf/progs/strobemeta.h | 5 +-
tools/testing/selftests/bpf/test_sysctl.c | 1 -
10 files changed, 138 insertions(+), 35 deletions(-)
^ permalink raw reply
* Re: [PATCH bpf-next] tools: libbpf: update extended attributes version of bpf_object__open()
From: KP Singh @ 2019-09-27 13:12 UTC (permalink / raw)
To: Song Liu
Cc: Anton Protopopov, Andrii Nakryiko, Alexei Starovoitov,
Daniel Borkmann, Martin Lau, Yonghong Song, Networking, bpf,
linux-kernel@vger.kernel.org
In-Reply-To: <9EC54605-1911-48B0-B33A-02EC46DEF3DD@fb.com>
On 30-Aug 19:24, Song Liu wrote:
>
>
> > On Aug 30, 2019, at 11:53 AM, Anton Protopopov <a.s.protopopov@gmail.com> wrote:
> >
> > чт, 29 авг. 2019 г. в 16:02, Song Liu <songliubraving@fb.com>:
> >>
> >>
> >>
> >>> On Aug 14, 2019, at 5:03 PM, Anton Protopopov <a.s.protopopov@gmail.com> wrote:
> >>>
> >>
> >> [...]
> >>
> >>>
> >>>
> >>> int bpf_object__unload(struct bpf_object *obj)
> >>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> >>> index e8f70977d137..634f278578dd 100644
> >>> --- a/tools/lib/bpf/libbpf.h
> >>> +++ b/tools/lib/bpf/libbpf.h
> >>> @@ -63,8 +63,13 @@ LIBBPF_API libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn);
> >>> struct bpf_object;
> >>>
> >>> struct bpf_object_open_attr {
> >>> - const char *file;
> >>> + union {
> >>> + const char *file;
> >>> + const char *obj_name;
> >>> + };
> >>> enum bpf_prog_type prog_type;
> >>> + void *obj_buf;
> >>> + size_t obj_buf_sz;
> >>> };
> >>
> >> I think this would break dynamically linked libbpf. No?
> >
> > Ah, yes, sure. What is the right way to make changes which break ABI in libbpf?
>
> I don't have a good idea here on the top of my head.
>
> Maybe we need a new struct and/or function for this.
I incorporated the suggested fixes and sent a new patch for this as we
ran into pretty much the same issue. (i.e. not being able to set
needs_kver / flags).
https://lore.kernel.org/bpf/20190927130834.18829-1-kpsingh@chromium.org/T/#u
- KP
>
> >
> > BTW, does the commit ddc7c3042614 ("libbpf: implement BPF CO-RE offset
> > relocation algorithm") which adds a new field to the struct
> > bpf_object_load_attr also break ABI?
>
> I think this change was in the same release, so it is OK.
>
> Thanks,
> Song
^ permalink raw reply
* Re: [PATCH net] devlink: Fix error handling in param and info_get dumpit cb
From: Jiri Pirko @ 2019-09-27 13:08 UTC (permalink / raw)
To: Andrew Lunn
Cc: Vasundhara Volam, David Miller, Netdev, Jiri Pirko, Michael Chan
In-Reply-To: <20190927123129.GB25474@lunn.ch>
Fri, Sep 27, 2019 at 02:31:29PM CEST, andrew@lunn.ch wrote:
>On Fri, Sep 27, 2019 at 10:28:36AM +0530, Vasundhara Volam wrote:
>> On Thu, Sep 26, 2019 at 5:57 PM Andrew Lunn <andrew@lunn.ch> wrote:
>> >
>> > On Thu, Sep 26, 2019 at 03:05:54PM +0530, Vasundhara Volam wrote:
>> > > If any of the param or info_get op returns error, dumpit cb is
>> > > skipping to dump remaining params or info_get ops for all the
>> > > drivers.
>> > >
>> > > Instead skip only for the param/info_get op which returned error
>> > > and continue to dump remaining information, except if the return
>> > > code is EMSGSIZE.
>> >
>> > Hi Vasundhara
>> >
>> > How do we get to see something did fail? If it failed, it failed for a
>> > reason, and we want to know.
>> >
>> > What is your real use case here? What is failing, and why are you
>> > O.K. to skip this failure?
>> >
>> > Andrew
>> Hi Andrew,
>>
>> Thank you for looking into the patch.
>>
>> If any of the devlink parameter is returning error like EINVAL, then
>> current code is not displaying any further parameters for all the other
>> devices as well.
>>
>> In bnxt_en driver case, some of the parameters are not supported in
>> certain configurations like if the parameter is not part of the
>> NVM configuration, driver returns EINVAL error to the stack. And devlink is
>> skipping to display all the remaining parameters for that device and others
>> as well.
>>
>> I am trying to fix to skip only the error parameter and display the remaining
>> parameters.
>
>Hi Vasundhara
>
>Thanks for explaining your use case. It sounds sensible. But i would
>narrow this down.
>
>Make the driver return EOPNOTSUP, not EINVAL. And then in dump, only
>skip EOPNOTSUP. Any other errors cause the error to be returned, so we
>get to see them.
Agreed, that would be more reasonable.
>
> Andrew
^ permalink raw reply
* Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS
From: Neil Horman @ 2019-09-27 12:51 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm, dwalsh, mpatel
In-Reply-To: <230e91cd3e50a3d8015daac135c24c4c58cf0a21.1568834524.git.rgb@redhat.com>
On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote:
> Set an arbitrary limit on the number of audit container identifiers to
> limit abuse.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit.c | 8 ++++++++
> kernel/audit.h | 4 ++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 53d13d638c63..329916534dd2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -139,6 +139,7 @@ struct audit_net {
> struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
> /* Hash for contid-based rules */
> struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
> +int audit_contid_count = 0;
>
> static struct kmem_cache *audit_buffer_cache;
>
> @@ -2384,6 +2385,7 @@ void audit_cont_put(struct audit_cont *cont)
> put_task_struct(cont->owner);
> list_del_rcu(&cont->list);
> kfree_rcu(cont, rcu);
> + audit_contid_count--;
> }
> }
>
> @@ -2456,6 +2458,11 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> goto conterror;
> }
> }
> + /* Set max contids */
> + if (audit_contid_count > AUDIT_CONTID_COUNT) {
> + rc = -ENOSPC;
> + goto conterror;
> + }
You should check for audit_contid_count == AUDIT_CONTID_COUNT here, no?
or at least >=, since you increment it below. Otherwise its possible
that you will exceed it by one in the full condition.
> if (!newcont) {
> newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
> if (newcont) {
> @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> newcont->owner = current;
> refcount_set(&newcont->refcount, 1);
> list_add_rcu(&newcont->list, &audit_contid_hash[h]);
> + audit_contid_count++;
> } else {
> rc = -ENOMEM;
> goto conterror;
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 162de8366b32..543f1334ba47 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
> return (contid & (AUDIT_CONTID_BUCKETS-1));
> }
>
> +extern int audit_contid_count;
> +
> +#define AUDIT_CONTID_COUNT 1 << 16
> +
Just to ask the question, since it wasn't clear in the changelog, what
abuse are you avoiding here? Ostensibly you should be able to create as
many container ids as you have space for, and the simple creation of
container ids doesn't seem like the resource strain I would be concerned
about here, given that an orchestrator can still create as many
containers as the system will otherwise allow, which will consume
significantly more ram/disk/etc.
> /* Indicates that audit should log the full pathname. */
> #define AUDIT_NAME_FULL -1
>
> --
> 1.8.3.1
>
>
^ permalink raw reply
* Re: [PATCH bpf] libbpf: add macro __BUILD_STATIC_LIBBPF__ to guard .symver
From: Daniel Borkmann @ 2019-09-27 12:48 UTC (permalink / raw)
To: Yonghong Song
Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, kernel-team
In-Reply-To: <20190926230204.1911391-1-yhs@fb.com>
On Thu, Sep 26, 2019 at 04:02:04PM -0700, Yonghong Song wrote:
> bcc uses libbpf repo as a submodule. It brings in libbpf source
> code and builds everything together to produce shared libraries.
> With latest libbpf, I got the following errors:
> /bin/ld: libbcc_bpf.so.0.10.0: version node not found for symbol xsk_umem__create@LIBBPF_0.0.2
> /bin/ld: failed to set dynamic section sizes: Bad value
> collect2: error: ld returned 1 exit status
> make[2]: *** [src/cc/libbcc_bpf.so.0.10.0] Error 1
>
> In xsk.c, we have
> asm(".symver xsk_umem__create_v0_0_2, xsk_umem__create@LIBBPF_0.0.2");
> asm(".symver xsk_umem__create_v0_0_4, xsk_umem__create@@LIBBPF_0.0.4");
> The linker thinks the built is for LIBBPF but cannot find proper version
> LIBBPF_0.0.2/4, so emit errors.
>
> I also confirmed that using libbpf.a to produce a shared library also
> has issues:
> -bash-4.4$ cat t.c
> extern void *xsk_umem__create;
> void * test() { return xsk_umem__create; }
> -bash-4.4$ gcc -c t.c
> -bash-4.4$ gcc -shared t.o libbpf.a -o t.so
> /bin/ld: t.so: version node not found for symbol xsk_umem__create@LIBBPF_0.0.2
> /bin/ld: failed to set dynamic section sizes: Bad value
> collect2: error: ld returned 1 exit status
> -bash-4.4$
>
> To fix the problem, I simply added a macro __BUILD_STATIC_LIBBPF__
> which will prevent issuing .symver assembly codes when enabled.
> The .symver assembly codes are still issued by default.
> This will at least give other libbpf users to build libbpf
> without these versioned symbols.
>
> I did not touch Makefile to actually use this macro to build
> static library as I want to check whether this is desirable or not.
Isn't there any better way on how we can detect this? Asking users to
pass this macro to the build seems a suboptimal user experience. How
are other libraries solving this given this seems really not specific
to libbpf?
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> tools/lib/bpf/xsk.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index 24fa313524fb..76c12c4c5c70 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -261,8 +261,11 @@ int xsk_umem__create_v0_0_2(struct xsk_umem **umem_ptr, void *umem_area,
> return xsk_umem__create_v0_0_4(umem_ptr, umem_area, size, fill, comp,
> &config);
> }
> +
> +#ifndef __BUILD_STATIC_LIBBPF__
> asm(".symver xsk_umem__create_v0_0_2, xsk_umem__create@LIBBPF_0.0.2");
> asm(".symver xsk_umem__create_v0_0_4, xsk_umem__create@@LIBBPF_0.0.4");
> +#endif
>
> static int xsk_load_xdp_prog(struct xsk_socket *xsk)
> {
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH] vhost: introduce mdev based hardware backend
From: Michael S. Tsirkin @ 2019-09-27 12:46 UTC (permalink / raw)
To: Jason Wang
Cc: Tiwei Bie, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang,
lingshan.zhu
In-Reply-To: <a959fe1e-3095-e0f0-0c9b-57f6eaa9c8b7@redhat.com>
On Fri, Sep 27, 2019 at 08:17:47PM +0800, Jason Wang wrote:
>
> On 2019/9/27 下午5:41, Michael S. Tsirkin wrote:
> > On Fri, Sep 27, 2019 at 11:27:12AM +0800, Jason Wang wrote:
> > > On 2019/9/26 下午9:14, Tiwei Bie wrote:
> > > > On Thu, Sep 26, 2019 at 04:35:18AM -0400, Michael S. Tsirkin wrote:
> > > > > On Thu, Sep 26, 2019 at 12:54:27PM +0800, Tiwei Bie wrote:
> > > > [...]
> > > > > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > > > > index 40d028eed645..5afbc2f08fa3 100644
> > > > > > --- a/include/uapi/linux/vhost.h
> > > > > > +++ b/include/uapi/linux/vhost.h
> > > > > > @@ -116,4 +116,12 @@
> > > > > > #define VHOST_VSOCK_SET_GUEST_CID _IOW(VHOST_VIRTIO, 0x60, __u64)
> > > > > > #define VHOST_VSOCK_SET_RUNNING _IOW(VHOST_VIRTIO, 0x61, int)
> > > > > > +/* VHOST_MDEV specific defines */
> > > > > > +
> > > > > > +#define VHOST_MDEV_SET_STATE _IOW(VHOST_VIRTIO, 0x70, __u64)
> > > > > > +
> > > > > > +#define VHOST_MDEV_S_STOPPED 0
> > > > > > +#define VHOST_MDEV_S_RUNNING 1
> > > > > > +#define VHOST_MDEV_S_MAX 2
> > > > > > +
> > > > > > #endif
> > > > > So assuming we have an underlying device that behaves like virtio:
> > > > I think they are really good questions/suggestions. Thanks!
> > > >
> > > > > 1. Should we use SET_STATUS maybe?
> > > > I like this idea. I will give it a try.
> > > >
> > > > > 2. Do we want a reset ioctl?
> > > > I think it is helpful. If we use SET_STATUS, maybe we
> > > > can use it to support the reset.
> > > >
> > > > > 3. Do we want ability to enable rings individually?
> > > > I will make it possible at least in the vhost layer.
> > >
> > > Note the API support e.g set_vq_ready().
> > virtio spec calls this "enabled" so let's stick to that.
>
>
> Ok.
>
>
> >
> > > > > 4. Does device need to limit max ring size?
> > > > > 5. Does device need to limit max number of queues?
> > > > I think so. It's helpful to have ioctls to report the max
> > > > ring size and max number of queues.
> > >
> > > An issue is the max number of queues is done through a device specific way,
> > > usually device configuration space. This is supported by the transport API,
> > > but how to expose it to userspace may need more thought.
> > >
> > > Thanks
> > an ioctl for device config? But for v1 I'd be quite happy to just have
> > a minimal working device with 2 queues.
>
>
> I'm fully agree, and it will work as long as VIRTIO_NET_F_MQ and
> VIRTIO_NET_F_CTRL_VQ is not advertised by the mdev device.
>
> Thanks
Hmm this means we need to validate the features bits,
not just pass them through to the hardware.
Problem is, how do we add more feature bits later,
without testing all hardware?
I guess this means the device specific driver must do it.
>
> >
> > > > Thanks!
^ permalink raw reply
* Re: [PATCH 1/1] bpf: Fix bpf_event_output re-entry issue
From: Daniel Borkmann @ 2019-09-27 12:44 UTC (permalink / raw)
To: Allan Zhang
Cc: songliubraving, netdev, bpf, linux-kernel, Stanislav Fomichev,
Eric Dumazet
In-Reply-To: <20190925234312.94063-2-allanzhang@google.com>
On Wed, Sep 25, 2019 at 04:43:12PM -0700, Allan Zhang wrote:
> BPF_PROG_TYPE_SOCK_OPS program can reenter bpf_event_output because it can
> be called from atomic and non-atomic contexts since we don't have
> bpf_prog_active to prevent it happen.
>
> This patch enables 3 level of nesting to support normal, irq and nmi
> context.
>
> We can easily reproduce the issue by running neper crr mode with 100 flows
> and 10 threads from neper client side.
>
> Here is the whole stack dump:
[...]
>
> Fixes: a5a3a828cd00 ("bpf: add perf event notificaton support for sock_ops")
>
> Effort: BPF
> Signed-off-by: Allan Zhang <allanzhang@google.com>
> Reviewed-by: Stanislav Fomichev <sdf@google.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH net] tcp: better handle TCP_USER_TIMEOUT in SYN_SENT state
From: Marek Majkowski @ 2019-09-27 12:40 UTC (permalink / raw)
To: edumazet
Cc: davem, jmaxwell37, eric.dumazet, marek, netdev, ycheng,
kernel-team
In-Reply-To: <CAGHK07B9E0AOBNtqVqKyJQOdU7ijdVi-7jLwnH+=S7ZgG5kpeA@mail.gmail.com>
On 9/27/19 10:25 AM, Jonathan Maxwell wrote:
> Acked-by: Jon Maxwell <jmaxwell37@gmail.com>
>
> Thanks for fixing that Eric.
>
The patch seems to do the job.
Tested-by: Marek Majkowski <marek@cloudflare.com>
Here's a selftest:
---8<---
From: Marek Majkowski <marek@cloudflare.com>
Date: Fri, 27 Sep 2019 13:37:52 +0200
Subject: [PATCH] selftests/net: TCP_USER_TIMEOUT in SYN-SENT state
Test the TCP_USER_TIMEOUT behavior, overriding TCP_SYNCNT
when socket is in SYN-SENT state.
Signed-off-by: Marek Majkowski <marek@cloudflare.com>
---
tools/testing/selftests/net/.gitignore | 1 +
tools/testing/selftests/net/Makefile | 3 +-
.../selftests/net/tcp_user_timeout_syn_sent.c | 322 ++++++++++++++++++
.../net/tcp_user_timeout_syn_sent.sh | 4 +
4 files changed, 329 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/net/tcp_user_timeout_syn_sent.c
create mode 100755 tools/testing/selftests/net/tcp_user_timeout_syn_sent.sh
diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
index c7cced739c34..bc6a2b7199b6 100644
--- a/tools/testing/selftests/net/.gitignore
+++ b/tools/testing/selftests/net/.gitignore
@@ -21,3 +21,4 @@ ipv6_flowlabel
ipv6_flowlabel_mgr
so_txtime
tcp_fastopen_backup_key
+tcp_user_timeout_syn_sent
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 0bd6b23c97ef..065a171b8834 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -11,13 +11,14 @@ TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh
TEST_PROGS += udpgro_bench.sh udpgro.sh test_vxlan_under_vrf.sh reuseport_addr_any.sh
TEST_PROGS += test_vxlan_fdb_changelink.sh so_txtime.sh ipv6_flowlabel.sh
TEST_PROGS += tcp_fastopen_backup_key.sh fcnal-test.sh l2tp.sh
+TEST_PROGS += tcp_user_timeout_syn_sent.sh
TEST_PROGS_EXTENDED := in_netns.sh
TEST_GEN_FILES = socket nettest
TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any
TEST_GEN_FILES += tcp_mmap tcp_inq psock_snd txring_overwrite
TEST_GEN_FILES += udpgso udpgso_bench_tx udpgso_bench_rx ip_defrag
TEST_GEN_FILES += so_txtime ipv6_flowlabel ipv6_flowlabel_mgr
-TEST_GEN_FILES += tcp_fastopen_backup_key
+TEST_GEN_FILES += tcp_fastopen_backup_key tcp_user_timeout_syn_sent
TEST_GEN_PROGS = reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
TEST_GEN_PROGS += reuseport_dualstack reuseaddr_conflict tls
diff --git a/tools/testing/selftests/net/tcp_user_timeout_syn_sent.c b/tools/testing/selftests/net/tcp_user_timeout_syn_sent.c
new file mode 100644
index 000000000000..1c9ec582359a
--- /dev/null
+++ b/tools/testing/selftests/net/tcp_user_timeout_syn_sent.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Testing if TCP_USER_TIMEOUT on SYN-SENT state overides TCP_SYNCNT.
+ *
+ * Historically, TCP_USER_TIMEOUT made only sense on synchronized TCP
+ * states, like ESTABLISHED. There was a bug on SYN-SENT state: with
+ * TCP_USER_TIMEOUT set, the connect() would ETIMEDOUT after given
+ * time, but near the end of the timer would flood SYN packets to
+ * fulfill the TCP_SYNCNT counter. For example for 2000ms user
+ * timeout and default TCP_SYNCNT=6, the tcpdump would look like:
+ *
+ * 00:00.000000 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
+ * 00:01.029452 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
+ * 00:02.021354 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
+ * 00:02.033419 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
+ * 00:02.041633 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
+ * 00:02.049263 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
+ * 00:02.057264 IP 127.0.0.1.1 > 127.0.0.1.2: Flags [S]
+ *
+ * Notice, 5 out of 6 retransmissions are aligned to 2s. We fixed
+ * that, and this code tests for the regression. We do this by
+ * actively dropping SYN packets on listen socket with ebpf
+ * SOCKET_FILTER, and counting how many packets did we drop.
+ *
+ * See: https://blog.cloudflare.com/when-tcp-sockets-refuse-to-die/
+ */
+#include <arpa/inet.h>
+#include <errno.h>
+#include <error.h>
+#include <linux/bpf.h>
+#include <linux/tcp.h>
+#include <linux/unistd.h>
+#include <netinet/in.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+int bpf_create_map(enum bpf_map_type map_type, int key_size, int value_size,
+ int max_entries, uint32_t map_flags)
+{
+ union bpf_attr attr = {};
+
+ attr.map_type = map_type;
+ attr.key_size = key_size;
+ attr.value_size = value_size;
+ attr.max_entries = max_entries;
+ attr.map_flags = map_flags;
+ return syscall(__NR_bpf, BPF_MAP_CREATE, &attr, sizeof(attr));
+}
+
+int bpf_load_program(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
+ size_t insns_cnt, const char *license,
+ uint32_t kern_version)
+{
+ union bpf_attr attr = {};
+
+ attr.prog_type = prog_type;
+ attr.insns = (long)insns;
+ attr.insn_cnt = insns_cnt;
+ attr.license = (long)license;
+ attr.log_buf = (long)NULL;
+ attr.log_size = 0;
+ attr.log_level = 0;
+ attr.kern_version = kern_version;
+
+ int fd = syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr));
+ return fd;
+}
+
+int bpf_map_update_elem(int fd, const void *key, const void *value,
+ uint64_t flags)
+{
+ union bpf_attr attr = {};
+
+ attr.map_fd = fd;
+ attr.key = (long)key;
+ attr.value = (long)value;
+ attr.flags = flags;
+
+ return syscall(__NR_bpf, BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr));
+}
+
+int bpf_map_lookup_elem(int fd, const void *key, void *value)
+{
+ union bpf_attr attr = {};
+
+ attr.map_fd = fd;
+ attr.key = (long)key;
+ attr.value = (long)value;
+
+ return syscall(__NR_bpf, BPF_MAP_LOOKUP_ELEM, &attr, sizeof(attr));
+}
+
+/*
+struct bpf_map_def SEC("maps") stats_map = {
+ .type = BPF_MAP_TYPE_ARRAY,
+ .key_size = sizeof(uint32_t),
+ .value_size = sizeof(uint64_t),
+ .max_entries = 2,
+};
+
+SEC("socket_filter")
+int _socket_filter(struct __sk_buff *skb)
+{
+ (void)skb;
+
+ uint32_t no = 0;
+ uint64_t *value = bpf_map_lookup_elem(&stats_map, &no);
+ if (value) {
+ __sync_fetch_and_add(value, 1);
+ }
+ return 0; // DROP inbound SYN packets
+}
+ */
+
+size_t bpf_insn_socket_filter_cnt = 12;
+struct bpf_insn bpf_insn_socket_filter[] = {
+ {
+ .code = 0xb7,
+ .dst_reg = BPF_REG_1,
+ .src_reg = BPF_REG_0,
+ .off = 0,
+ .imm = 0 /**/
+ },
+ {
+ .code = 0x63,
+ .dst_reg = BPF_REG_10,
+ .src_reg = BPF_REG_1,
+ .off = -4,
+ .imm = 0 /**/
+ },
+ {
+ .code = 0xbf,
+ .dst_reg = BPF_REG_2,
+ .src_reg = BPF_REG_10,
+ .off = 0,
+ .imm = 0 /**/
+ },
+ {
+ .code = 0x7,
+ .dst_reg = BPF_REG_2,
+ .src_reg = BPF_REG_0,
+ .off = 0,
+ .imm = -4 /**/
+ },
+ {
+ .code = 0x18,
+ .dst_reg = BPF_REG_1,
+ .src_reg = BPF_REG_0,
+ .off = 0,
+ .imm = 0 /* relocation for stats_map */
+ },
+ {
+ .code = 0x0,
+ .dst_reg = BPF_REG_0,
+ .src_reg = BPF_REG_0,
+ .off = 0,
+ .imm = 0 /**/
+ },
+ {
+ .code = 0x85,
+ .dst_reg = BPF_REG_0,
+ .src_reg = BPF_REG_0,
+ .off = 0,
+ .imm = 1 /**/
+ },
+ {
+ .code = 0x15,
+ .dst_reg = BPF_REG_0,
+ .src_reg = BPF_REG_0,
+ .off = 2,
+ .imm = 0 /**/
+ },
+ {
+ .code = 0xb7,
+ .dst_reg = BPF_REG_1,
+ .src_reg = BPF_REG_0,
+ .off = 0,
+ .imm = 1 /**/
+ },
+ {
+ .code = 0xdb,
+ .dst_reg = BPF_REG_0,
+ .src_reg = BPF_REG_1,
+ .off = 0,
+ .imm = 0 /**/
+ },
+ {
+ .code = 0xb7,
+ .dst_reg = BPF_REG_0,
+ .src_reg = BPF_REG_0,
+ .off = 0,
+ .imm = 0 /**/
+ },
+ {
+ .code = 0x95,
+ .dst_reg = BPF_REG_0,
+ .src_reg = BPF_REG_0,
+ .off = 0,
+ .imm = 0 /**/
+ }
+};
+
+void socket_filter_fill_stats_map(int fd)
+{
+ bpf_insn_socket_filter[4].src_reg = BPF_REG_1;
+ bpf_insn_socket_filter[4].imm = fd;
+}
+
+static int net_setup_ebpf(int sd)
+{
+ int stats_map, bpf, r;
+
+ stats_map = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(uint32_t),
+ sizeof(uint64_t), 2, 0);
+ if (stats_map < 0)
+ error(1, errno, "bpf");
+
+ socket_filter_fill_stats_map(stats_map);
+
+ bpf = bpf_load_program(BPF_PROG_TYPE_SOCKET_FILTER,
+ bpf_insn_socket_filter,
+ bpf_insn_socket_filter_cnt, "Dual BSD/GPL", 0);
+ if (bpf < 0)
+ error(1, errno, "bpf");
+
+ r = setsockopt(sd, SOL_SOCKET, SO_ATTACH_BPF, &bpf, sizeof(bpf));
+ if (r < 0)
+ error(1, errno, "setsockopt(SO_ATTACH_FILTER)");
+
+ return stats_map;
+}
+
+static int setup_server(struct sockaddr_in *addr)
+{
+ int sd, r;
+ socklen_t addr_sz;
+
+ sd = socket(AF_INET, SOCK_STREAM, 0);
+ if (sd < 0)
+ error(1, errno, "socket()");
+
+ r = bind(sd, (struct sockaddr *)addr, sizeof(*addr));
+ if (r != 0)
+ error(1, errno, "bind()");
+
+ r = listen(sd, 16);
+ if (r != 0)
+ error(1, errno, "listen()");
+
+ addr_sz = sizeof(*addr);
+ r = getsockname(sd, (struct sockaddr *)addr, &addr_sz);
+ if (r != 0)
+ error(1, errno, "getsockname()");
+
+ return sd;
+}
+
+int main(void)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_addr = { inet_addr("127.0.0.1") },
+ };
+
+ int sd = setup_server(&addr);
+ int stats_map = net_setup_ebpf(sd);
+ struct {
+ int user_timeout;
+ int expected_counter;
+ } tests[] = {
+ { 200, 2 }, // TCP_USER_TIMEOUT kicks in on first retranmission
+ { 1500, 2 },
+ { 3500, 3 },
+ { -1, -1 },
+ };
+
+ int failed = 0, i;
+
+ for (i = 0; tests[i].user_timeout >= 0; i++) {
+ int r, cd, v;
+ uint32_t k = 0;
+ uint64_t counter = 0;
+
+ r = bpf_map_update_elem(stats_map, &k, &counter, 0);
+
+ cd = socket(AF_INET, SOCK_STREAM, 0);
+ if (cd < 0)
+ error(1, errno, "socket()");
+
+ v = tests[i].user_timeout;
+ r = setsockopt(cd, IPPROTO_TCP, TCP_USER_TIMEOUT, &v,
+ sizeof(v));
+ if (r != 0)
+ error(1, errno, "setsockopt()");
+
+ r = connect(cd, (struct sockaddr *)&addr, sizeof(addr));
+ if (r != -1 && errno != ETIMEDOUT)
+ error(1, errno, "connect()");
+
+ r = bpf_map_lookup_elem(stats_map, &k, &counter);
+ if (r != 0)
+ error(1, errno, "bpf_map_lookup_elem()");
+
+ if ((int)counter != tests[i].expected_counter) {
+ failed += 1;
+ printf("[!] Expecting %d SYN packets on "
+ "TCP_USER_TIMEOUT=%d, got %d\n",
+ tests[i].expected_counter, tests[i].user_timeout,
+ (int)counter);
+ }
+ close(cd);
+ }
+ close(sd);
+ close(stats_map);
+ if (failed == 0)
+ fprintf(stderr, "PASSED\n");
+ return failed;
+}
diff --git a/tools/testing/selftests/net/tcp_user_timeout_syn_sent.sh b/tools/testing/selftests/net/tcp_user_timeout_syn_sent.sh
new file mode 100755
index 000000000000..26765f3a92c6
--- /dev/null
+++ b/tools/testing/selftests/net/tcp_user_timeout_syn_sent.sh
@@ -0,0 +1,4 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+./in_netns.sh ./tcp_user_timeout_syn_sent
--
2.17.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox