Netdev List
 help / color / mirror / Atom feed
* Re: r8169 not working on 5.2.0rc6 with GPD MicroPC
From: Karsten Wiborg @ 2019-07-01 18:15 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit; +Cc: nic_swsd, romieu, netdev
In-Reply-To: <20190701133507.GB25795@lunn.ch>

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]

Hi Andrew, Heiner,

the device is a really small notebook. So detaching mains still leaves
the battery which is delicately built in. So can't currently remove
power completely.

Anyway can I deliver more debugging data to you guys which might add
support for the r8169 for this device?

Regards,
Karsten

On 01/07/2019 15:35, Andrew Lunn wrote:
>> When the vendor driver assigns a random MAC address, it writes it to the
>> chip. The related registers may be persistent (can't say exactly due to
>> missing documentation).
> 
> If the device supports WOL, it could be it is powered using the
> standby supply, not the main supply. Try pulling the plug from the
> wall to really remove all power.
> 
>      Andrew
> 


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4156 bytes --]

^ permalink raw reply

* [PATCH rdma-next 2/2] IB/mlx5: Implement VHCA tunnel mechanism in DEVX
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>

This mechanism will allow function-A to perform operations "on behalf"
of function-B via tunnel object. Function-A will have privileges for
creating and using this tunnel object.

For example, in the device emulation feature presented in Bluefield-1 SoC,
using device emulation capability, one can present NVMe function to the host OS.

Since the NVMe function doesn't have a normal command interface to the HCA HW,
here is a need to create a channel that will be able to issue commands "on behalf"
of this function.

This channel is the VHCA_TUNNEL general object. The emulation software will create
this tunnel for every managed function and issue commands via devx general cmd
interface using the appropriate tunnel ID. When devX context will receive a command
with non-zero vhca_tunnel_id, it will pass the command as-is down to the HCA.

All the validation, security and resource tracking of the commands and the created
tunneled objects is in the responsibility of the HCA FW. When a VHCA_TUNNEL object
destroyed, the device will issue an internal FLR (function level reset) to the
emulated function associated with this tunnel. This will destroy all the created
resources using the tunnel mechanism.

Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
Reviewed-by: Yishai Hadas <yishaih@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/hw/mlx5/devx.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
index cb4f0fc79176..26ba6bf5d19a 100644
--- a/drivers/infiniband/hw/mlx5/devx.c
+++ b/drivers/infiniband/hw/mlx5/devx.c
@@ -894,12 +894,16 @@ static int devx_get_uid(struct mlx5_ib_ucontext *c, void *cmd_in)
 
 	return c->devx_uid;
 }
-static bool devx_is_general_cmd(void *in)
+
+static bool devx_is_general_cmd(void *in, struct mlx5_ib_dev *dev)
 {
 	u16 opcode = MLX5_GET(general_obj_in_cmd_hdr, in, opcode);
 
-	if (opcode >= MLX5_CMD_OP_GENERAL_START &&
-	    opcode < MLX5_CMD_OP_GENERAL_END)
+	/* Pass all cmds for vhca_tunnel as general, tracking is done in FW */
+	if ((MLX5_CAP_GEN_64(dev->mdev, vhca_tunnel_commands) &&
+	     MLX5_GET(general_obj_in_cmd_hdr, in, vhca_tunnel_id)) ||
+	    (opcode >= MLX5_CMD_OP_GENERAL_START &&
+	     opcode < MLX5_CMD_OP_GENERAL_END))
 		return true;
 
 	switch (opcode) {
@@ -1025,7 +1029,7 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OTHER)(
 		return uid;
 
 	/* Only white list of some general HCA commands are allowed for this method. */
-	if (!devx_is_general_cmd(cmd_in))
+	if (!devx_is_general_cmd(cmd_in, dev))
 		return -EINVAL;
 
 	cmd_out = uverbs_zalloc(attrs, cmd_out_len);
@@ -1420,6 +1424,9 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_CREATE)(
 	u32 obj_id;
 	u16 opcode;
 
+	if (MLX5_GET(general_obj_in_cmd_hdr, cmd_in, vhca_tunnel_id))
+		return -EINVAL;
+
 	uid = devx_get_uid(c, cmd_in);
 	if (uid < 0)
 		return uid;
@@ -1519,6 +1526,9 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_MODIFY)(
 	int err;
 	int uid;
 
+	if (MLX5_GET(general_obj_in_cmd_hdr, cmd_in, vhca_tunnel_id))
+		return -EINVAL;
+
 	uid = devx_get_uid(c, cmd_in);
 	if (uid < 0)
 		return uid;
@@ -1561,6 +1571,9 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_QUERY)(
 	int uid;
 	struct mlx5_ib_dev *mdev = to_mdev(c->ibucontext.device);
 
+	if (MLX5_GET(general_obj_in_cmd_hdr, cmd_in, vhca_tunnel_id))
+		return -EINVAL;
+
 	uid = devx_get_uid(c, cmd_in);
 	if (uid < 0)
 		return uid;
@@ -1698,6 +1711,9 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_ASYNC_QUERY)(
 	struct devx_async_cmd_event_file *ev_file;
 	struct devx_async_data *async_data;
 
+	if (MLX5_GET(general_obj_in_cmd_hdr, cmd_in, vhca_tunnel_id))
+		return -EINVAL;
+
 	uid = devx_get_uid(c, cmd_in);
 	if (uid < 0)
 		return uid;
-- 
2.20.1


^ permalink raw reply related

* [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


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox