* [PATCH mlx5-next 1/2] net/mlx5: Introduce VHCA tunnel device capability
From: Leon Romanovsky @ 2019-07-01 18:14 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Max Gurtovoy, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190701181402.25286-1-leon@kernel.org>
From: Max Gurtovoy <maxg@mellanox.com>
When using the device emulation feature (introduced in Bluefield-1 SOC),
a privileged function (the device emulation manager) will be able to
create a channel to execute commands on behalf of the emulated function.
This channel will be a general object of type VHCA_TUNNEL that will have
a unique ID for each emulated function. This ID will be passed in each
cmd that will be issued by the emulation SW in a well known offset in
the command header.
This channel is needed since the emulated function doesn't have a normal
command interface to the HCA HW, but some basic configuration for that
function is needed (e.g. initialize and enable the HCA). For that matter,
a specific command-set was defined and only those commands will be issued
by the HCA.
Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
Reviewed-by: Yishai Hadas <yishaih@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
include/linux/mlx5/mlx5_ifc.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 5d2bf91e130b..a35e545a8de0 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -1329,7 +1329,13 @@ struct mlx5_ifc_cmd_hca_cap_bits {
u8 reserved_at_6c0[0x4];
u8 flex_parser_id_geneve_tlv_option_0[0x4];
- u8 reserved_at_6c8[0x138];
+ u8 reserved_at_6c8[0x18];
+
+ u8 reserved_at_6e0[0xa0];
+
+ u8 vhca_tunnel_commands[0x40];
+
+ u8 reserved_at_7c0[0x40];
};
enum mlx5_flow_destination_type {
@@ -9560,7 +9566,7 @@ struct mlx5_ifc_general_obj_in_cmd_hdr_bits {
u8 opcode[0x10];
u8 uid[0x10];
- u8 reserved_at_20[0x10];
+ u8 vhca_tunnel_id[0x10];
u8 obj_type[0x10];
u8 obj_id[0x20];
--
2.20.1
^ permalink raw reply related
* [PATCH rdma-next 0/2] DEVX VHCA tunnel support
From: Leon Romanovsky @ 2019-07-01 18:14 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, RDMA mailing list, Max Gurtovoy, Yishai Hadas,
Saeed Mahameed, linux-netdev
From: Leon Romanovsky <leonro@mellanox.com>
Hi,
Those two patches introduce VHCA tunnel mechanism to DEVX interface
needed for Bluefield SOC. See extensive commit messages for more
information.
Thanks
Max Gurtovoy (2):
net/mlx5: Introduce VHCA tunnel device capability
IB/mlx5: Implement VHCA tunnel mechanism in DEVX
drivers/infiniband/hw/mlx5/devx.c | 24 ++++++++++++++++++++----
include/linux/mlx5/mlx5_ifc.h | 10 ++++++++--
2 files changed, 28 insertions(+), 6 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCH v5 net-next 6/6] net: ethernet: ti: cpsw: add XDP support
From: Ilias Apalodimas @ 2019-07-01 18:09 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Ivan Khoronzhuk, grygorii.strashko, davem, ast, linux-kernel,
linux-omap, netdev, daniel, jakub.kicinski, john.fastabend
In-Reply-To: <20190701181901.150c0b71@carbon>
On Mon, Jul 01, 2019 at 06:19:01PM +0200, Jesper Dangaard Brouer wrote:
> On Sun, 30 Jun 2019 20:23:48 +0300
> Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
> > +static int cpsw_ndev_create_xdp_rxq(struct cpsw_priv *priv, int ch)
> > +{
> > + struct cpsw_common *cpsw = priv->cpsw;
> > + int ret, new_pool = false;
> > + struct xdp_rxq_info *rxq;
> > +
> > + rxq = &priv->xdp_rxq[ch];
> > +
> > + ret = xdp_rxq_info_reg(rxq, priv->ndev, ch);
> > + if (ret)
> > + return ret;
> > +
> > + if (!cpsw->page_pool[ch]) {
> > + ret = cpsw_create_rx_pool(cpsw, ch);
> > + if (ret)
> > + goto err_rxq;
> > +
> > + new_pool = true;
> > + }
> > +
> > + ret = xdp_rxq_info_reg_mem_model(rxq, MEM_TYPE_PAGE_POOL,
> > + cpsw->page_pool[ch]);
> > + if (!ret)
> > + return 0;
> > +
> > + if (new_pool) {
> > + page_pool_free(cpsw->page_pool[ch]);
> > + cpsw->page_pool[ch] = NULL;
> > + }
> > +
> > +err_rxq:
> > + xdp_rxq_info_unreg(rxq);
> > + return ret;
> > +}
>
> Looking at this, and Ilias'es XDP-netsec error handling path, it might
> be a mistake that I removed page_pool_destroy() and instead put the
> responsibility on xdp_rxq_info_unreg().
>
> As here, we have to detect if page_pool_create() was a success, and then
> if xdp_rxq_info_reg_mem_model() was a failure, explicitly call
> page_pool_free() because the xdp_rxq_info_unreg() call cannot "free"
> the page_pool object given it was not registered.
>
> Ivan's patch in[1], might be a better approach, which forced all
> drivers to explicitly call page_pool_free(), even-though it just
> dec-refcnt and the real call to page_pool_free() happened via
> xdp_rxq_info_unreg().
We did discuss that xdp_XXXXX naming might be confusing.
That being said since Ivan's approach serves 'special' hardware and fixes the
naming irregularity, i perfectly fine doing that as long as we clearly document
that the API is supposed to serve a pool per queue (unless the hardware needs to
deal with it differently)
>
> To better handle error path, I would re-introduce page_pool_destroy(),
> as a driver API, that would gracefully handle NULL-pointer case, and
> then call page_pool_free() with the atomic_dec_and_test(). (It should
> hopefully simplify the error handling code a bit)
>
> [1] https://lore.kernel.org/netdev/20190625175948.24771-2-ivan.khoronzhuk@linaro.org/
>
>
Thanks
/Ilias
> > +void cpsw_ndev_destroy_xdp_rxqs(struct cpsw_priv *priv)
> > +{
> > + struct cpsw_common *cpsw = priv->cpsw;
> > + struct xdp_rxq_info *rxq;
> > + int i;
> > +
> > + for (i = 0; i < cpsw->rx_ch_num; i++) {
> > + rxq = &priv->xdp_rxq[i];
> > + if (xdp_rxq_info_is_reg(rxq))
> > + xdp_rxq_info_unreg(rxq);
> > + }
> > +}
>
> Are you sure you need to test xdp_rxq_info_is_reg() here?
>
> You should just call xdp_rxq_info_unreg(rxq), if you know that this rxq
> should be registered. If your assumption failed, you will get a
> WARNing, and discover your driver level bug. This is one of the ways
> the API is designed to "detect" misuse of the API. (I found this
> rather useful, when I converted the approx 12 drivers using this
> xdp_rxq_info API).
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH v2 net-next 16/19] ionic: Add driver stats
From: Shannon Nelson @ 2019-07-01 18:06 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev
In-Reply-To: <20190629115324.7adfc3c9@cakuba.netronome.com>
On 6/29/19 11:53 AM, Jakub Kicinski wrote:
> On Fri, 28 Jun 2019 14:39:31 -0700, Shannon Nelson wrote:
>> Add in the detailed statistics for ethtool -S that the driver
>> keeps as it processes packets. Display of the additional
>> debug statistics can be enabled through the ethtool priv-flags
>> feature.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>
>> +static void ionic_get_strings(struct net_device *netdev,
>> + u32 sset, u8 *buf)
>> +{
>> + struct lif *lif = netdev_priv(netdev);
>> +
>> + switch (sset) {
>> + case ETH_SS_STATS:
>> + ionic_get_stats_strings(lif, buf);
>> + break;
>> + case ETH_SS_PRIV_FLAGS:
>> + memcpy(buf, ionic_priv_flags_strings,
>> + PRIV_FLAGS_COUNT * ETH_GSTRING_LEN);
>> + break;
>> + case ETH_SS_TEST:
>> + // IONIC_TODO
>> + default:
>> + netdev_err(netdev, "Invalid sset %d\n", sset);
> Not really an error, as long as sset_count() returns a 0 nothing will
> happen. Also you can drop the SS_TEST if you don't report it.
Sure.
>> +
>> +#endif // _IONIC_STATS_H_
> Perhaps worth grepping the driver for C++ style comments?
Those nasty little things sneak in there when you're not looking, and
you try to get rid of them all, and there always seems to be one more
:-). Yes, I'll look again.
sln
^ permalink raw reply
* Re: net: check before dereferencing netdev_ops during busy poll
From: Matteo Croce @ 2019-07-01 18:03 UTC (permalink / raw)
To: Josh Elsasser
Cc: Greg Kroah-Hartman, Sasha Levin, stable, netdev, LKML,
David Miller
In-Reply-To: <20190701175241.GB9081@kroah.com>
On Mon, Jul 1, 2019 at 7:53 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sat, Jun 29, 2019 at 09:39:39PM +0200, Matteo Croce wrote:
> > On Sat, Jun 29, 2019 at 9:45 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Jun 28, 2019 at 07:03:01PM -0700, Josh Elsasser wrote:
> > > > On Jun 28, 2019, at 3:55 PM, Sasha Levin <sashal@kernel.org> wrote:
> > > >
> > > > > What's the upstream commit id?
> > > >
> > > > The commit wasn't needed upstream, as I only sent the original patch after
> > > > 79e7fff47b7b ("net: remove support for per driver ndo_busy_poll()") had
> > > > made the fix unnecessary in Linus' tree.
> > > >
> > > > May've gotten lost in the shuffle due to my poor Fixes tags. The patch in
> > > > question applied only on top of the 4.9 stable release at the time, but the
> > > > actual NPE had been around in some form since 3.11 / 0602129286705 ("net: add
> > > > low latency socket poll").
> > >
> > > Ok, can people then resend this and be very explicit as to why this is
> > > needed only in a stable kernel tree and get reviews from people agreeing
> > > that this really is the correct fix?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > Hi Greg,
> >
> > I think that David alredy reviewed the patch here:
> >
> > https://lore.kernel.org/netdev/20180313.105115.682846171057663636.davem@davemloft.net/
> >
> > Anyway, I tested the patch and it fixes the panic, at least on my
> > iwlwifi card, so:
> >
> > Tested-by: Matteo Croce <mcroce@redhat.com>
>
> Ok, but what can I do with this? I need a real patch, in mail form,
> that I can apply. Not a web link to an email archive.
>
> You have read the stable kernel rules, right? :)
>
> greg k-h
Understood.
Josh, as you are the original author, can you please resend it to -stable?
Feel free to add this tag:
Tested-by: Matteo Croce <mcroce@redhat.com>
Regards,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* Re: [PATCH v2 net-next 17/19] ionic: Add RSS support
From: Shannon Nelson @ 2019-07-01 18:03 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev
In-Reply-To: <20190629114839.6cf1f048@cakuba.netronome.com>
On 6/29/19 11:48 AM, Jakub Kicinski wrote:
> On Fri, 28 Jun 2019 14:39:32 -0700, Shannon Nelson wrote:
>> @@ -1260,10 +1266,24 @@ static struct lif *ionic_lif_alloc(struct ionic *ionic, unsigned int index)
>> if (err)
>> goto err_out_free_lif_info;
>>
>> + /* allocate rss indirection table */
>> + tbl_sz = le16_to_cpu(lif->ionic->ident.lif.eth.rss_ind_tbl_sz);
>> + lif->rss_ind_tbl_sz = sizeof(*lif->rss_ind_tbl) * tbl_sz;
>> + lif->rss_ind_tbl = dma_alloc_coherent(dev, lif->rss_ind_tbl_sz,
>> + &lif->rss_ind_tbl_pa,
>> + GFP_KERNEL);
>> +
>> + if (!lif->rss_ind_tbl) {
>> + dev_err(dev, "Failed to allocate rss indirection table, aborting\n");
>> + goto err_out_free_qcqs;
>> + }
>> +
>> list_add_tail(&lif->list, &ionic->lifs);
>>
>> return lif;
>>
>> +err_out_free_qcqs:
>> + ionic_qcqs_free(lif);
>> err_out_free_lif_info:
>> dma_free_coherent(dev, lif->info_sz, lif->info, lif->info_pa);
>> lif->info = NULL;
>> @@ -1302,6 +1322,14 @@ static void ionic_lif_free(struct lif *lif)
>> {
>> struct device *dev = lif->ionic->dev;
>>
>> + /* free rss indirection table */
>> + if (lif->rss_ind_tbl) {
>> + dma_free_coherent(dev, lif->rss_ind_tbl_sz, lif->rss_ind_tbl,
>> + lif->rss_ind_tbl_pa);
> dma_free_coherent() should be able to deal with NULLs just fine.
> Besides you fail hard if the allocation failed, no?
I like checking my pointers, but sure, this can be simplified.
sln
>
>> + lif->rss_ind_tbl = NULL;
>> + lif->rss_ind_tbl_pa = 0;
>> + }
^ permalink raw reply
* Re: [PATCH 2/2] samples: pktgen: allow to specify destination port
From: David Miller @ 2019-07-01 18:02 UTC (permalink / raw)
To: danieltimlee; +Cc: brouer, netdev
In-Reply-To: <20190629133358.8251-2-danieltimlee@gmail.com>
From: "Daniel T. Lee" <danieltimlee@gmail.com>
Date: Sat, 29 Jun 2019 22:33:58 +0900
> Currently, kernel pktgen has the feature to specify udp destination port
> for sending packet. (e.g. pgset "udp_dst_min 9")
>
> But on samples, each of the scripts doesn't have any option to achieve this.
>
> This commit adds the DST_PORT option to specify the target port(s) in the script.
>
> -p : ($DST_PORT) destination PORT range (e.g. 433-444) is also allowed
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH 1/2] samples: pktgen: add some helper functions for port parsing
From: David Miller @ 2019-07-01 18:02 UTC (permalink / raw)
To: danieltimlee; +Cc: brouer, netdev
In-Reply-To: <20190629133358.8251-1-danieltimlee@gmail.com>
From: "Daniel T. Lee" <danieltimlee@gmail.com>
Date: Sat, 29 Jun 2019 22:33:57 +0900
> This commit adds port parsing and port validate helper function to parse
> single or range of port(s) from a given string. (e.g. 1234, 443-444)
>
> Helpers will be used in prior to set target port(s) in samples/pktgen.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
Applied.
^ permalink raw reply
* Re: 答复: [PATCH v4] net: netfilter: Fix rpfilter dropping vrf packets by mistake
From: Pablo Neira Ayuso @ 2019-07-01 18:01 UTC (permalink / raw)
To: linmiaohe
Cc: David Ahern, kadlec@blackhole.kfki.hu, fw@strlen.de,
davem@davemloft.net, kuznet@ms2.inr.ac.ru,
yoshfuji@linux-ipv6.org, netfilter-devel@vger.kernel.org,
coreteam@netfilter.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Mingfangsen
In-Reply-To: <cef929f9a14f462f9f7d3fa475f84e76@huawei.com>
On Sat, Jun 29, 2019 at 02:13:59PM +0000, linmiaohe wrote:
> On 6/29/19 20:20 PM, David Ahern wrote:
> > On 6/28/19 8:13 PM, linmiaohe wrote:
> > > You're right. Fib rules code would set FLOWI_FLAG_SKIP_NH_OIF flag.
> > > But I set it here for distinguish with the flags & XT_RPFILTER_LOOSE
> > > branch. Without this, they do the same work and maybe should be
> > > combined. I don't want to do that as that makes code confusing.
> > > Is this code snipet below ok ? If so, I would delete this flag setting.
> > >
> > > } else if (netif_is_l3_master(dev) || netif_is_l3_slave(dev)) {
> > > fl6.flowi6_oif = dev->ifindex;
> > > } else if ((flags & XT_RPFILTER_LOOSE) == 0)
> > > fl6.flowi6_oif = dev->ifindex;
>
> > that looks fine to me, but it is up to Pablo.
>
> @David Ahern Many thanks for your valuable advice.
>
> @ Pablo Hi, could you please tell me if this code snipet is ok?
> If not, which code would you prefer? It's very nice of you to
> figure it out for me. Thanks a lot.
Probably this?
} else if (netif_is_l3_master(dev) || netif_is_l3_slave(dev) ||
(flags & XT_RPFILTER_LOOSE) == 0) {
fl6.flowi6_oif = dev->ifindex;
}
Thanks.
^ permalink raw reply
* Re: [PATCH v2 net-next 04/19] ionic: Add basic lif support
From: Shannon Nelson @ 2019-07-01 18:00 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev
In-Reply-To: <20190629114526.4da68321@cakuba.netronome.com>
On 6/29/19 11:45 AM, Jakub Kicinski wrote:
> On Fri, 28 Jun 2019 14:39:19 -0700, Shannon Nelson wrote:
>> @@ -64,4 +66,49 @@ int ionic_debugfs_add_ident(struct ionic *ionic)
>> ionic, &identity_fops) ? 0 : -ENOTSUPP;
>> }
>>
>> +int ionic_debugfs_add_sizes(struct ionic *ionic)
>> +{
>> + debugfs_create_u32("nlifs", 0400, ionic->dentry,
>> + (u32 *)&ionic->ident.dev.nlifs);
>> + debugfs_create_u32("nintrs", 0400, ionic->dentry, &ionic->nintrs);
>> +
>> + debugfs_create_u32("ntxqs_per_lif", 0400, ionic->dentry,
>> + (u32 *)&ionic->ident.lif.eth.config.queue_count[IONIC_QTYPE_TXQ]);
>> + debugfs_create_u32("nrxqs_per_lif", 0400, ionic->dentry,
>> + (u32 *)&ionic->ident.lif.eth.config.queue_count[IONIC_QTYPE_RXQ]);
> Are these __le32s? Does the driver build cleanly with W=1 C=1?
Aside from a couple of "expression using sizeof(void)" messages that I
haven't been able to fully clean out, yes, the driver builds pretty
clean with sparse. In this particular case, I think the typecast stops
the complaint that you are expecting here. But yes, these would be
better off with le32_to_cpu().
sln
>
>> + return 0;
>> +}
^ permalink raw reply
* Re: [PATCH] net:gue.h:Fix shifting signed 32-bit value by 31 bits problem
From: David Miller @ 2019-07-01 18:00 UTC (permalink / raw)
To: bnvandana; +Cc: netdev, linux-kernel, skhan, gregkh, linux-kernel-mentees
In-Reply-To: <20190701141610.3681-1-bnvandana@gmail.com>
From: Vandana BN <bnvandana@gmail.com>
Date: Mon, 1 Jul 2019 19:46:10 +0530
> Fix GUE_PFLAG_REMCSUM to use "U" cast to avoid shifting signed
> 32-bit value by 31 bits problem.
>
> Signed-off-by: Vandana BN <bnvandana@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: icmp: allow flowlabel reflection in echo replies
From: David Miller @ 2019-07-01 17:59 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet
In-Reply-To: <20190701133936.15238-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Mon, 1 Jul 2019 06:39:36 -0700
> Extend flowlabel_reflect bitmask to allow conditional
> reflection of incoming flowlabels in echo replies.
>
> Note this has precedence against auto flowlabels.
>
> Add flowlabel_reflect enum to replace hard coded
> values.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: net: check before dereferencing netdev_ops during busy poll
From: Greg Kroah-Hartman @ 2019-07-01 17:52 UTC (permalink / raw)
To: Matteo Croce
Cc: Josh Elsasser, Sasha Levin, stable, netdev, LKML, David Miller
In-Reply-To: <CAGnkfhzmGbeQe7L55nEv575XyubWqCLz=7NQPpH+TajDkkDiXg@mail.gmail.com>
On Sat, Jun 29, 2019 at 09:39:39PM +0200, Matteo Croce wrote:
> On Sat, Jun 29, 2019 at 9:45 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Jun 28, 2019 at 07:03:01PM -0700, Josh Elsasser wrote:
> > > On Jun 28, 2019, at 3:55 PM, Sasha Levin <sashal@kernel.org> wrote:
> > >
> > > > What's the upstream commit id?
> > >
> > > The commit wasn't needed upstream, as I only sent the original patch after
> > > 79e7fff47b7b ("net: remove support for per driver ndo_busy_poll()") had
> > > made the fix unnecessary in Linus' tree.
> > >
> > > May've gotten lost in the shuffle due to my poor Fixes tags. The patch in
> > > question applied only on top of the 4.9 stable release at the time, but the
> > > actual NPE had been around in some form since 3.11 / 0602129286705 ("net: add
> > > low latency socket poll").
> >
> > Ok, can people then resend this and be very explicit as to why this is
> > needed only in a stable kernel tree and get reviews from people agreeing
> > that this really is the correct fix?
> >
> > thanks,
> >
> > greg k-h
>
> Hi Greg,
>
> I think that David alredy reviewed the patch here:
>
> https://lore.kernel.org/netdev/20180313.105115.682846171057663636.davem@davemloft.net/
>
> Anyway, I tested the patch and it fixes the panic, at least on my
> iwlwifi card, so:
>
> Tested-by: Matteo Croce <mcroce@redhat.com>
Ok, but what can I do with this? I need a real patch, in mail form,
that I can apply. Not a web link to an email archive.
You have read the stable kernel rules, right? :)
greg k-h
^ permalink raw reply
* Re: net: check before dereferencing netdev_ops during busy poll
From: Greg Kroah-Hartman @ 2019-07-01 17:52 UTC (permalink / raw)
To: Matteo Croce; +Cc: stable, Sasha Levin, Josh Elsasser, netdev, LKML
In-Reply-To: <CAGnkfhxxw9keiNj_Qm=2GBYpY38HAq28cOROMRqXfbqq8wNbWQ@mail.gmail.com>
On Fri, Jun 28, 2019 at 06:34:58PM +0200, Matteo Croce wrote:
> Hi,
>
> Is there any reason for this panic fix not being applied in stable?
>
> https://lore.kernel.org/netdev/20180313053248.13654-1-jelsasser@appneta.com/T/
I can't apply patches from random urls :)
^ permalink raw reply
* Re: [PATCH bpf-next v3 3/3] selftests/bpf: add verifier tests for wide stores
From: Yonghong Song @ 2019-07-01 17:50 UTC (permalink / raw)
To: Stanislav Fomichev, netdev@vger.kernel.org, bpf@vger.kernel.org
Cc: davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
Andrii Nakryiko
In-Reply-To: <20190701173841.32249-4-sdf@google.com>
On 7/1/19 10:38 AM, Stanislav Fomichev wrote:
> Make sure that wide stores are allowed at proper (aligned) addresses.
> Note that user_ip6 is naturally aligned on 8-byte boundary, so
> correct addresses are user_ip6[0] and user_ip6[2]. msg_src_ip6 is,
> however, aligned on a 4-byte bondary, so only msg_src_ip6[1]
> can be wide-stored.
>
> Cc: Andrii Nakryiko <andriin@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [PATCH] net: dst.h: Fix shifting signed 32-bit value by 31 bits problem
From: David Miller @ 2019-07-01 17:49 UTC (permalink / raw)
To: bnvandana; +Cc: netdev, linux-kernel, skhan, gregkh, linux-kernel-mentees
In-Reply-To: <20190701115539.6738-1-bnvandana@gmail.com>
From: Vandana BN <bnvandana@gmail.com>
Date: Mon, 1 Jul 2019 17:25:39 +0530
> Fix DST_FEATURE_ECN_CA to use "U" cast to avoid shifting signed
> 32-bit value by 31 bits problem.
>
> Signed-off-by: Vandana BN <bnvandana@gmail.com>
Applied.
^ permalink raw reply
* [PATCH net-next] bonding/main: fix NULL dereference in bond_select_active_slave()
From: Eric Dumazet @ 2019-07-01 17:48 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Eric Dumazet, John Sperbeck, Jarod Wilson,
Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
A bonding master can be up while best_slave is NULL.
[12105.636318] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
[12105.638204] mlx4_en: eth1: Linkstate event 1 -> 1
[12105.648984] IP: bond_select_active_slave+0x125/0x250
[12105.653977] PGD 0 P4D 0
[12105.656572] Oops: 0000 [#1] SMP PTI
[12105.660487] gsmi: Log Shutdown Reason 0x03
[12105.664620] Modules linked in: kvm_intel loop act_mirred uhaul vfat fat stg_standard_ftl stg_megablocks stg_idt stg_hdi stg elephant_dev_num stg_idt_eeprom w1_therm wire i2c_mux_pca954x i2c_mux mlx4_i2c i2c_usb cdc_acm ehci_pci ehci_hcd i2c_iimc mlx4_en mlx4_ib ib_uverbs ib_core mlx4_core [last unloaded: kvm_intel]
[12105.685686] mlx4_core 0000:03:00.0: dispatching link up event for port 2
[12105.685700] mlx4_en: eth2: Linkstate event 2 -> 1
[12105.685700] mlx4_en: eth2: Link Up (linkstate)
[12105.724452] Workqueue: bond0 bond_mii_monitor
[12105.728854] RIP: 0010:bond_select_active_slave+0x125/0x250
[12105.734355] RSP: 0018:ffffaf146a81fd88 EFLAGS: 00010246
[12105.739637] RAX: 0000000000000003 RBX: ffff8c62b03c6900 RCX: 0000000000000000
[12105.746838] RDX: 0000000000000000 RSI: ffffaf146a81fd08 RDI: ffff8c62b03c6000
[12105.754054] RBP: ffffaf146a81fdb8 R08: 0000000000000001 R09: ffff8c517d387600
[12105.761299] R10: 00000000001075d9 R11: ffffffffaceba92f R12: 0000000000000000
[12105.768553] R13: ffff8c8240ae4800 R14: 0000000000000000 R15: 0000000000000000
[12105.775748] FS: 0000000000000000(0000) GS:ffff8c62bfa40000(0000) knlGS:0000000000000000
[12105.783892] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[12105.789716] CR2: 0000000000000000 CR3: 0000000d0520e001 CR4: 00000000001626f0
[12105.796976] Call Trace:
[12105.799446] [<ffffffffac31d387>] bond_mii_monitor+0x497/0x6f0
[12105.805317] [<ffffffffabd42643>] process_one_work+0x143/0x370
[12105.811225] [<ffffffffabd42c7a>] worker_thread+0x4a/0x360
[12105.816761] [<ffffffffabd48bc5>] kthread+0x105/0x140
[12105.821865] [<ffffffffabd42c30>] ? rescuer_thread+0x380/0x380
[12105.827757] [<ffffffffabd48ac0>] ? kthread_associate_blkcg+0xc0/0xc0
[12105.834266] [<ffffffffac600241>] ret_from_fork+0x51/0x60
Fixes: e2a7420df2e0 ("bonding/main: convert to using slave printk macros")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: John Sperbeck <jsperbeck@google.com>
Cc: Jarod Wilson <jarod@redhat.com>
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
---
drivers/net/bonding/bond_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a30595955a37a485b9e045a31969313f8336b668..84168455aded96dfd85b310841dee2a0d917b580 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -937,7 +937,7 @@ void bond_select_active_slave(struct bonding *bond)
return;
if (netif_carrier_ok(bond->dev))
- slave_info(bond->dev, best_slave->dev, "active interface up!\n");
+ netdev_info(bond->dev, "active interface up!\n");
else
netdev_info(bond->dev, "now running without any active interface!\n");
}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* Re: [PATCH net-next v2 00/10] net: stmmac: 10GbE using XGMAC
From: David Miller @ 2019-07-01 17:46 UTC (permalink / raw)
To: Jose.Abreu
Cc: linux-kernel, netdev, Joao.Pinto, peppe.cavallaro,
alexandre.torgue
In-Reply-To: <BN8PR12MB32662DA0B5733E93D88E7D7DD3F90@BN8PR12MB3266.namprd12.prod.outlook.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Mon, 1 Jul 2019 10:19:55 +0000
> From: David Miller <davem@davemloft.net>
>
>> About the Kconfig change, maybe it just doesn't make sense to list all
>> of the various speeds the chip supports... just a thought.
>
> What about: "STMicroelectronics Multi-Gigabit Ethernet driver" ?
>
> Or, just "STMicroelectronics Ethernet driver" ?
Maybe the first one is better, I don't know. What are the chances of there
being more STMicroelectronics ethernet NICs? :-)
^ permalink raw reply
* Re: [PATCH net] Documentation/networking: fix default_ttl typo in mpls-sysctl
From: David Miller @ 2019-07-01 17:42 UTC (permalink / raw)
To: liuhangbin; +Cc: netdev
In-Reply-To: <20190701084528.25872-1-liuhangbin@gmail.com>
From: Hangbin Liu <liuhangbin@gmail.com>
Date: Mon, 1 Jul 2019 16:45:28 +0800
> default_ttl should be integer instead of bool
>
> Reported-by: Ying Xu <yinxu@redhat.com>
> Fixes: a59166e47086 ("mpls: allow TTL propagation from IP packets to be configured")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Yonghong Song @ 2019-07-01 17:40 UTC (permalink / raw)
To: Stanislav Fomichev, Andrii Nakryiko
Cc: Stanislav Fomichev, netdev@vger.kernel.org, bpf@vger.kernel.org,
davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
Andrii Nakryiko, kernel test robot
In-Reply-To: <20190701160434.GD6757@mini-arch>
On 7/1/19 9:04 AM, Stanislav Fomichev wrote:
> On 07/01, Andrii Nakryiko wrote:
>> On Sat, Jun 29, 2019 at 10:53 PM Yonghong Song <yhs@fb.com> wrote:
>>>
>>>
>>>
>>> On 6/28/19 4:10 PM, Stanislav Fomichev wrote:
>>>> Since commit cd17d7770578 ("bpf/tools: sync bpf.h") clang decided
>>>> that it can do a single u64 store into user_ip6[2] instead of two
>>>> separate u32 ones:
>>>>
>>>> # 17: (18) r2 = 0x100000000000000
>>>> # ; ctx->user_ip6[2] = bpf_htonl(DST_REWRITE_IP6_2);
>>>> # 19: (7b) *(u64 *)(r1 +16) = r2
>>>> # invalid bpf_context access off=16 size=8
>>>>
>>>> From the compiler point of view it does look like a correct thing
>>>> to do, so let's support it on the kernel side.
>>>>
>>>> Credit to Andrii Nakryiko for a proper implementation of
>>>> bpf_ctx_wide_store_ok.
>>>>
>>>> Cc: Andrii Nakryiko <andriin@fb.com>
>>>> Cc: Yonghong Song <yhs@fb.com>
>>>> Fixes: cd17d7770578 ("bpf/tools: sync bpf.h")
>>>> Reported-by: kernel test robot <rong.a.chen@intel.com>
>>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>>
>>> The change looks good to me with the following nits:
>>> 1. could you add a cover letter for the patch set?
>>> typically if the number of patches is more than one,
>>> it would be a good practice with a cover letter.
>>> See bpf_devel_QA.rst .
>>> 2. with this change, the comments in uapi bpf.h
>>> are not accurate any more.
>>> __u32 user_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
>>> * Stored in network byte order.
>>>
>>> */
>>> __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
>>> * Stored in network byte order.
>>> */
>>> now for stores, aligned 8-byte write is permitted.
>>> could you update this as well?
>>>
>>> From the typical usage pattern, I did not see a need
>>> for 8-tye read of user_ip6 and msg_src_ip6 yet. So let
>>> us just deal with write for now.
>>
>> But I guess it's still possible for clang to optimize two consecutive
>> 4-byte reads into single 8-byte read in some circumstances? If that's
>> the case, maybe it's a good idea to have corresponding read checks as
>> well?
> I guess clang can do those kinds of optimizations. I can put it on my
> todo and address later (or when we actually see it out in the wild).
Okay, I find a Facebook internal app. does trying to read the 4 bytes
and compare to a predefined loopback address. We may need to handle
read cases as well. But this can be a followup after actual tryout.
>
>> But overall this looks good to me:
>>
>> Acked-by: Andrii Nakryiko <andriin@fb.com>
> Thanks for a review!
>
>>>
>>> With the above two nits,
>>> Acked-by: Yonghong Song <yhs@fb.com>
>>>
>>>> ---
>>>> include/linux/filter.h | 6 ++++++
>>>> net/core/filter.c | 22 ++++++++++++++--------
>>>> 2 files changed, 20 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>>>> index 340f7d648974..3901007e36f1 100644
>>>> --- a/include/linux/filter.h
>>>> +++ b/include/linux/filter.h
>>>> @@ -746,6 +746,12 @@ bpf_ctx_narrow_access_ok(u32 off, u32 size, u32 size_default)
>>>> return size <= size_default && (size & (size - 1)) == 0;
>>>> }
>>>>
>>>> +#define bpf_ctx_wide_store_ok(off, size, type, field) \
>>>> + (size == sizeof(__u64) && \
>>>> + off >= offsetof(type, field) && \
>>>> + off + sizeof(__u64) <= offsetofend(type, field) && \
>>>> + off % sizeof(__u64) == 0)
>>>> +
>>>> #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
>>>>
>>>> static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index dc8534be12fc..5d33f2146dab 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>> @@ -6849,6 +6849,16 @@ static bool sock_addr_is_valid_access(int off, int size,
>>>> if (!bpf_ctx_narrow_access_ok(off, size, size_default))
>>>> return false;
>>>> } else {
>>>> + if (bpf_ctx_wide_store_ok(off, size,
>>>> + struct bpf_sock_addr,
>>>> + user_ip6))
>>>> + return true;
>>>> +
>>>> + if (bpf_ctx_wide_store_ok(off, size,
>>>> + struct bpf_sock_addr,
>>>> + msg_src_ip6))
>>>> + return true;
>>>> +
>>>> if (size != size_default)
>>>> return false;
>>>> }
>>>> @@ -7689,9 +7699,6 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
>>>> /* SOCK_ADDR_STORE_NESTED_FIELD_OFF() has semantic similar to
>>>> * SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF() but for store operation.
>>>> *
>>>> - * It doesn't support SIZE argument though since narrow stores are not
>>>> - * supported for now.
>>>> - *
>>>> * In addition it uses Temporary Field TF (member of struct S) as the 3rd
>>>> * "register" since two registers available in convert_ctx_access are not
>>>> * enough: we can't override neither SRC, since it contains value to store, nor
>>>> @@ -7699,7 +7706,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
>>>> * instructions. But we need a temporary place to save pointer to nested
>>>> * structure whose field we want to store to.
>>>> */
>>>> -#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, TF) \
>>>> +#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, OFF, TF) \
>>>> do { \
>>>> int tmp_reg = BPF_REG_9; \
>>>> if (si->src_reg == tmp_reg || si->dst_reg == tmp_reg) \
>>>> @@ -7710,8 +7717,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
>>>> offsetof(S, TF)); \
>>>> *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(S, F), tmp_reg, \
>>>> si->dst_reg, offsetof(S, F)); \
>>>> - *insn++ = BPF_STX_MEM( \
>>>> - BPF_FIELD_SIZEOF(NS, NF), tmp_reg, si->src_reg, \
>>>> + *insn++ = BPF_STX_MEM(SIZE, tmp_reg, si->src_reg, \
>>>> bpf_target_off(NS, NF, FIELD_SIZEOF(NS, NF), \
>>>> target_size) \
>>>> + OFF); \
>>>> @@ -7723,8 +7729,8 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
>>>> TF) \
>>>> do { \
>>>> if (type == BPF_WRITE) { \
>>>> - SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, \
>>>> - TF); \
>>>> + SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, \
>>>> + OFF, TF); \
>>>> } else { \
>>>> SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF( \
>>>> S, NS, F, NF, SIZE, OFF); \
>>>>
^ permalink raw reply
* [PATCH bpf-next v3 3/3] selftests/bpf: add verifier tests for wide stores
From: Stanislav Fomichev @ 2019-07-01 17:38 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko,
Yonghong Song
In-Reply-To: <20190701173841.32249-1-sdf@google.com>
Make sure that wide stores are allowed at proper (aligned) addresses.
Note that user_ip6 is naturally aligned on 8-byte boundary, so
correct addresses are user_ip6[0] and user_ip6[2]. msg_src_ip6 is,
however, aligned on a 4-byte bondary, so only msg_src_ip6[1]
can be wide-stored.
Cc: Andrii Nakryiko <andriin@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/test_verifier.c | 17 +++++++--
.../selftests/bpf/verifier/wide_store.c | 36 +++++++++++++++++++
2 files changed, 50 insertions(+), 3 deletions(-)
create mode 100644 tools/testing/selftests/bpf/verifier/wide_store.c
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index c5514daf8865..b0773291012a 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -105,6 +105,7 @@ struct bpf_test {
__u64 data64[TEST_DATA_LEN / 8];
};
} retvals[MAX_TEST_RUNS];
+ enum bpf_attach_type expected_attach_type;
};
/* Note we want this to be 64 bit aligned so that the end of our array is
@@ -850,6 +851,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
int fd_prog, expected_ret, alignment_prevented_execution;
int prog_len, prog_type = test->prog_type;
struct bpf_insn *prog = test->insns;
+ struct bpf_load_program_attr attr;
int run_errs, run_successes;
int map_fds[MAX_NR_MAPS];
const char *expected_err;
@@ -881,8 +883,17 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
pflags |= BPF_F_STRICT_ALIGNMENT;
if (test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS)
pflags |= BPF_F_ANY_ALIGNMENT;
- fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
- "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
+
+ memset(&attr, 0, sizeof(attr));
+ attr.prog_type = prog_type;
+ attr.expected_attach_type = test->expected_attach_type;
+ attr.insns = prog;
+ attr.insns_cnt = prog_len;
+ attr.license = "GPL";
+ attr.log_level = 4;
+ attr.prog_flags = pflags;
+
+ fd_prog = bpf_load_program_xattr(&attr, bpf_vlog, sizeof(bpf_vlog));
if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
printf("SKIP (unsupported program type %d)\n", prog_type);
skips++;
@@ -912,7 +923,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
printf("FAIL\nUnexpected success to load!\n");
goto fail_log;
}
- if (!strstr(bpf_vlog, expected_err)) {
+ if (!expected_err || !strstr(bpf_vlog, expected_err)) {
printf("FAIL\nUnexpected error message!\n\tEXP: %s\n\tRES: %s\n",
expected_err, bpf_vlog);
goto fail_log;
diff --git a/tools/testing/selftests/bpf/verifier/wide_store.c b/tools/testing/selftests/bpf/verifier/wide_store.c
new file mode 100644
index 000000000000..8fe99602ded4
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/wide_store.c
@@ -0,0 +1,36 @@
+#define BPF_SOCK_ADDR(field, off, res, err) \
+{ \
+ "wide store to bpf_sock_addr." #field "[" #off "]", \
+ .insns = { \
+ BPF_MOV64_IMM(BPF_REG_0, 1), \
+ BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, \
+ offsetof(struct bpf_sock_addr, field[off])), \
+ BPF_EXIT_INSN(), \
+ }, \
+ .result = res, \
+ .prog_type = BPF_PROG_TYPE_CGROUP_SOCK_ADDR, \
+ .expected_attach_type = BPF_CGROUP_UDP6_SENDMSG, \
+ .errstr = err, \
+}
+
+/* user_ip6[0] is u64 aligned */
+BPF_SOCK_ADDR(user_ip6, 0, ACCEPT,
+ NULL),
+BPF_SOCK_ADDR(user_ip6, 1, REJECT,
+ "invalid bpf_context access off=12 size=8"),
+BPF_SOCK_ADDR(user_ip6, 2, ACCEPT,
+ NULL),
+BPF_SOCK_ADDR(user_ip6, 3, REJECT,
+ "invalid bpf_context access off=20 size=8"),
+
+/* msg_src_ip6[0] is _not_ u64 aligned */
+BPF_SOCK_ADDR(msg_src_ip6, 0, REJECT,
+ "invalid bpf_context access off=44 size=8"),
+BPF_SOCK_ADDR(msg_src_ip6, 1, ACCEPT,
+ NULL),
+BPF_SOCK_ADDR(msg_src_ip6, 2, REJECT,
+ "invalid bpf_context access off=52 size=8"),
+BPF_SOCK_ADDR(msg_src_ip6, 3, REJECT,
+ "invalid bpf_context access off=56 size=8"),
+
+#undef BPF_SOCK_ADDR
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v3 2/3] bpf: sync bpf.h to tools/
From: Stanislav Fomichev @ 2019-07-01 17:38 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
In-Reply-To: <20190701173841.32249-1-sdf@google.com>
Sync user_ip6 & msg_src_ip6 comments.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/include/uapi/linux/bpf.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a396b516a2b2..c59dc921ce83 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3237,7 +3237,7 @@ struct bpf_sock_addr {
__u32 user_ip4; /* Allows 1,2,4-byte read and 4-byte write.
* Stored in network byte order.
*/
- __u32 user_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
+ __u32 user_ip6[4]; /* Allows 1,2,4-byte read and 4,8-byte write.
* Stored in network byte order.
*/
__u32 user_port; /* Allows 4-byte read and write.
@@ -3246,10 +3246,10 @@ struct bpf_sock_addr {
__u32 family; /* Allows 4-byte read, but no write */
__u32 type; /* Allows 4-byte read, but no write */
__u32 protocol; /* Allows 4-byte read, but no write */
- __u32 msg_src_ip4; /* Allows 1,2,4-byte read an 4-byte write.
+ __u32 msg_src_ip4; /* Allows 1,2,4-byte read and 4-byte write.
* Stored in network byte order.
*/
- __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
+ __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read and 4,8-byte write.
* Stored in network byte order.
*/
__bpf_md_ptr(struct bpf_sock *, sk);
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v3 1/3] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Stanislav Fomichev @ 2019-07-01 17:38 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko,
Yonghong Song, kernel test robot
In-Reply-To: <20190701173841.32249-1-sdf@google.com>
Since commit cd17d7770578 ("bpf/tools: sync bpf.h") clang decided
that it can do a single u64 store into user_ip6[2] instead of two
separate u32 ones:
# 17: (18) r2 = 0x100000000000000
# ; ctx->user_ip6[2] = bpf_htonl(DST_REWRITE_IP6_2);
# 19: (7b) *(u64 *)(r1 +16) = r2
# invalid bpf_context access off=16 size=8
From the compiler point of view it does look like a correct thing
to do, so let's support it on the kernel side.
Credit to Andrii Nakryiko for a proper implementation of
bpf_ctx_wide_store_ok.
Cc: Andrii Nakryiko <andriin@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Fixes: cd17d7770578 ("bpf/tools: sync bpf.h")
Reported-by: kernel test robot <rong.a.chen@intel.com>
Acked-by: Yonghong Song <yhs@fb.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/linux/filter.h | 6 ++++++
include/uapi/linux/bpf.h | 6 +++---
net/core/filter.c | 22 ++++++++++++++--------
3 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 1fe53e78c7e3..6d944369ca87 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -747,6 +747,12 @@ bpf_ctx_narrow_access_ok(u32 off, u32 size, u32 size_default)
return size <= size_default && (size & (size - 1)) == 0;
}
+#define bpf_ctx_wide_store_ok(off, size, type, field) \
+ (size == sizeof(__u64) && \
+ off >= offsetof(type, field) && \
+ off + sizeof(__u64) <= offsetofend(type, field) && \
+ off % sizeof(__u64) == 0)
+
#define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index cffea1826a1f..dcdc606e57d6 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3240,7 +3240,7 @@ struct bpf_sock_addr {
__u32 user_ip4; /* Allows 1,2,4-byte read and 4-byte write.
* Stored in network byte order.
*/
- __u32 user_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
+ __u32 user_ip6[4]; /* Allows 1,2,4-byte read and 4,8-byte write.
* Stored in network byte order.
*/
__u32 user_port; /* Allows 4-byte read and write.
@@ -3249,10 +3249,10 @@ struct bpf_sock_addr {
__u32 family; /* Allows 4-byte read, but no write */
__u32 type; /* Allows 4-byte read, but no write */
__u32 protocol; /* Allows 4-byte read, but no write */
- __u32 msg_src_ip4; /* Allows 1,2,4-byte read an 4-byte write.
+ __u32 msg_src_ip4; /* Allows 1,2,4-byte read and 4-byte write.
* Stored in network byte order.
*/
- __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read an 4-byte write.
+ __u32 msg_src_ip6[4]; /* Allows 1,2,4-byte read and 4,8-byte write.
* Stored in network byte order.
*/
__bpf_md_ptr(struct bpf_sock *, sk);
diff --git a/net/core/filter.c b/net/core/filter.c
index 4836264f82ee..2520dbc539fc 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6851,6 +6851,16 @@ static bool sock_addr_is_valid_access(int off, int size,
if (!bpf_ctx_narrow_access_ok(off, size, size_default))
return false;
} else {
+ if (bpf_ctx_wide_store_ok(off, size,
+ struct bpf_sock_addr,
+ user_ip6))
+ return true;
+
+ if (bpf_ctx_wide_store_ok(off, size,
+ struct bpf_sock_addr,
+ msg_src_ip6))
+ return true;
+
if (size != size_default)
return false;
}
@@ -7691,9 +7701,6 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
/* SOCK_ADDR_STORE_NESTED_FIELD_OFF() has semantic similar to
* SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF() but for store operation.
*
- * It doesn't support SIZE argument though since narrow stores are not
- * supported for now.
- *
* In addition it uses Temporary Field TF (member of struct S) as the 3rd
* "register" since two registers available in convert_ctx_access are not
* enough: we can't override neither SRC, since it contains value to store, nor
@@ -7701,7 +7708,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
* instructions. But we need a temporary place to save pointer to nested
* structure whose field we want to store to.
*/
-#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, TF) \
+#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, OFF, TF) \
do { \
int tmp_reg = BPF_REG_9; \
if (si->src_reg == tmp_reg || si->dst_reg == tmp_reg) \
@@ -7712,8 +7719,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
offsetof(S, TF)); \
*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(S, F), tmp_reg, \
si->dst_reg, offsetof(S, F)); \
- *insn++ = BPF_STX_MEM( \
- BPF_FIELD_SIZEOF(NS, NF), tmp_reg, si->src_reg, \
+ *insn++ = BPF_STX_MEM(SIZE, tmp_reg, si->src_reg, \
bpf_target_off(NS, NF, FIELD_SIZEOF(NS, NF), \
target_size) \
+ OFF); \
@@ -7725,8 +7731,8 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
TF) \
do { \
if (type == BPF_WRITE) { \
- SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, \
- TF); \
+ SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, \
+ OFF, TF); \
} else { \
SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF( \
S, NS, F, NF, SIZE, OFF); \
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v3 0/3] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Stanislav Fomichev @ 2019-07-01 17:38 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
Clang can generate 8-byte stores for user_ip6 & msg_src_ip6,
let's support that on the verifier side.
v3:
* fix comments spelling an -> and (Andrii Nakryiko)
v2:
* Add simple cover letter (Yonghong Song)
* Update comments (Yonghong Song)
* Remove [4] selftests (Yonghong Song)
Stanislav Fomichev (3):
bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
bpf: sync bpf.h to tools/
selftests/bpf: add verifier tests for wide stores
include/linux/filter.h | 6 ++++
include/uapi/linux/bpf.h | 6 ++--
net/core/filter.c | 22 +++++++-----
tools/include/uapi/linux/bpf.h | 6 ++--
tools/testing/selftests/bpf/test_verifier.c | 17 +++++++--
.../selftests/bpf/verifier/wide_store.c | 36 +++++++++++++++++++
6 files changed, 76 insertions(+), 17 deletions(-)
create mode 100644 tools/testing/selftests/bpf/verifier/wide_store.c
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: Use-after-free in br_multicast_rcv
From: Nikolay Aleksandrov @ 2019-07-01 17:37 UTC (permalink / raw)
To: Martin Weinelt, bridge, Roopa Prabhu; +Cc: netdev
In-Reply-To: <3fcf8b05-e1ad-ac97-10bf-bd2b6354424c@linuxlounge.net>
[-- Attachment #1: Type: text/plain, Size: 3413 bytes --]
Hi again,
On 01/07/2019 20:31, Martin Weinelt wrote:
> Hi Nik,
>
> On 7/1/19 7:03 PM, Nikolay Aleksandrov wrote:
>> Hi Martin,
>>
>> On 01/07/2019 19:53, Martin Weinelt wrote:
>>> Hi Nik,
>>>
>>> more info below.
>>>
>>> On 6/29/19 3:11 PM, nikolay@cumulusnetworks.com wrote:
>>>> On 29 June 2019 14:54:44 EEST, Martin Weinelt <martin@linuxlounge.net> wrote:
>>>>> Hello,
>>>>>
>>>>> we've recently been experiencing memory leaks on our Linux-based
>>>>> routers,
>>>>> at least as far back as v4.19.16.
>>>>>
>>>>> After rebuilding with KASAN it found a use-after-free in
>>>>> br_multicast_rcv which I could reproduce on v5.2.0-rc6.
>>>>>
>>>>> Please find the KASAN report below, I'm anot sure what else to provide
>>>>> so
>>>>> feel free to ask.
>>>>>
>>>>> Best,
>>>>> Martin
>>>>>
>>>>>
>>>>
>>>> Hi Martin,
>>>> I'll look into this, are there any specific steps to reproduce it?
>>>>
>>>> Thanks,
>>>> Nik
>>>>>
>>> Each server is a KVM Guest and has 18 bridges with the same master/slave
>>> relationships:
>>>
>>> bridge -> batman-adv -> {l2 tunnel, virtio device}
>>>
>>> Linus Lüssing from the batman-adv asked me to apply this patch to help
>>> debugging.
>>>
>>> v5.2-rc6-170-g728254541ebc with this patch yielded the following KASAN
>>> report, not sure if the additional information at the end is a result of
>>> the added patch though.
>>>
>>> Best,
>>> Martin
>>>
>>
>> I see a couple of issues that can cause out-of-bounds accesses in br_multicast.c
>> more specifically there're pskb_may_pull calls and accesses to stale skb pointers.
>> I've had these on my "to fix" list for some time now, will prepare, test the fixes and
>> send them for review. In a few minutes I'll send a test patch for you.
>> That being said, I thought you said you've been experiencing memory leaks, but below
>> reports are for out-of-bounds accesses, could you please clarify if you were
>> speaking about these or is there another issue as well ?
>> If you're experiencing memory leaks, are you sure they're related to the bridge ?
>> You could try kmemleak for those.
>>
>> Thank you,
>> Nik
>>
>
> we had been experiencing memory leaks on v4.19.37, thats why we started to turn on
> KASAN and kmemleak in the first place. This is when we found this use-after-free.
>
> The memory leak exists, and is a separate issue. Apparently kmemleak does not work,
> I suspect the early log size is too small
>
> root@gw02:~# echo scan > /sys/kernel/debug/kmemleak -bash: echo: write error: Device or resource busy
>
> CONFIG_HAVE_DEBUG_KMEMLEAK=y
> CONFIG_DEBUG_KMEMLEAK=y
> CONFIG_DEBUG_KMEMLEAK_EARLY_LOG_SIZE=400
> # CONFIG_DEBUG_KMEMLEAK_TEST is not set
> # CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF is not set
> CONFIG_DEBUG_KMEMLEAK_AUTO_SCAN=y
>
> I'll increase the early log size with the next build to try and get more information
> on the memory leak, I'll open a separate thread for that then.
>
> Thanks,
> Martin
>
I see, thanks for clarifying this. So on the KASAN could you please try the attached patch ?
Also could you please run the br_multicast_rcv+xxx addresses through
linux/scripts/faddr2line for your kernel/bridge:
usage: faddr2line [--list] <object file> <func+offset> <func+offset>...
Thanks,
Nik
[-- Attachment #2: 0001-net-bridge-mcast-fix-possible-uses-of-stale-pointers.patch --]
[-- Type: text/x-patch, Size: 3442 bytes --]
From 5358f2ad1228967d5e8a2dc21e0025651726a3b8 Mon Sep 17 00:00:00 2001
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Mon, 1 Jul 2019 20:31:14 +0300
Subject: [PATCH TEST] net: bridge: mcast: fix possible uses of stale pointers
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_multicast.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index de22c8fbbb15..fbedef3fa930 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -917,6 +917,8 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
len = skb_transport_offset(skb) + sizeof(*ih);
for (i = 0; i < num; i++) {
+ u16 nsrcs;
+
len += sizeof(*grec);
if (!ip_mc_may_pull(skb, len))
return -EINVAL;
@@ -924,8 +926,9 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
grec = (void *)(skb->data + len - sizeof(*grec));
group = grec->grec_mca;
type = grec->grec_type;
+ nsrcs = ntohs(grec->grec_nsrcs);
- len += ntohs(grec->grec_nsrcs) * 4;
+ len += nsrcs * 4;
if (!ip_mc_may_pull(skb, len))
return -EINVAL;
@@ -946,7 +949,7 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
src = eth_hdr(skb)->h_source;
if ((type == IGMPV3_CHANGE_TO_INCLUDE ||
type == IGMPV3_MODE_IS_INCLUDE) &&
- ntohs(grec->grec_nsrcs) == 0) {
+ nsrcs == 0) {
br_ip4_multicast_leave_group(br, port, group, vid, src);
} else {
err = br_ip4_multicast_add_group(br, port, group, vid,
@@ -983,7 +986,8 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
len = skb_transport_offset(skb) + sizeof(*icmp6h);
for (i = 0; i < num; i++) {
- __be16 *nsrcs, _nsrcs;
+ __be16 *_nsrcs, __nsrcs;
+ u16 nsrcs;
nsrcs_offset = len + offsetof(struct mld2_grec, grec_nsrcs);
@@ -991,12 +995,13 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
nsrcs_offset + sizeof(_nsrcs))
return -EINVAL;
- nsrcs = skb_header_pointer(skb, nsrcs_offset,
- sizeof(_nsrcs), &_nsrcs);
- if (!nsrcs)
+ _nsrcs = skb_header_pointer(skb, nsrcs_offset,
+ sizeof(_nsrcs), &__nsrcs);
+ if (!_nsrcs)
return -EINVAL;
- grec_len = struct_size(grec, grec_src, ntohs(*nsrcs));
+ nsrcs = ntohs(*_nsrcs);
+ grec_len = struct_size(grec, grec_src, nsrcs);
if (!ipv6_mc_may_pull(skb, len + grec_len))
return -EINVAL;
@@ -1021,7 +1026,7 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
src = eth_hdr(skb)->h_source;
if ((grec->grec_type == MLD2_CHANGE_TO_INCLUDE ||
grec->grec_type == MLD2_MODE_IS_INCLUDE) &&
- ntohs(*nsrcs) == 0) {
+ nsrcs == 0) {
br_ip6_multicast_leave_group(br, port, &grec->grec_mca,
vid, src);
} else {
@@ -1275,7 +1280,6 @@ static int br_ip6_multicast_query(struct net_bridge *br,
u16 vid)
{
unsigned int transport_len = ipv6_transport_len(skb);
- const struct ipv6hdr *ip6h = ipv6_hdr(skb);
struct mld_msg *mld;
struct net_bridge_mdb_entry *mp;
struct mld2_query *mld2q;
@@ -1319,7 +1323,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,
if (is_general_query) {
saddr.proto = htons(ETH_P_IPV6);
- saddr.u.ip6 = ip6h->saddr;
+ saddr.u.ip6 = ipv6_hdr(skb)->saddr;
br_multicast_query_received(br, port, &br->ip6_other_query,
&saddr, max_delay);
--
2.21.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox