* Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.
From: Andrew Lunn @ 2018-10-07 21:14 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, devicetree, David Miller, Florian Fainelli, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007205906.3h7fpyxdhc24o6lc@localhost>
On Sun, Oct 07, 2018 at 01:59:06PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 09:54:00PM +0200, Andrew Lunn wrote:
> > Sure, but things have moved on since then.
>
> If you have a specific suggestion on how to better implement this,
> please tell us what it is.
>
> > I can think of three obvious use cases where this does not work:
> >
> > 1) phylink, not phdev. We have been pushing some MAC drivers towards
> > phylink, especially those which support >1Gbp.
>
> If a phylink device appears that wants time stamping, can't we add the
> call to register_mii_timestamper()?
Hi Richard
The problem is you depend on skbuf->dev->phydev. phydev will be NULL.
net_device does not currently have a phylink member. Even if it did,
you end up add more and more tests looking every place a
mii_timestamper could be placed.
> > 2) When an SFP is connected to the MAC, not a copper PHY. The class of
> > device you are adding a driver for will work just as well for an SFP
> > as for a copper PHY. The SERDES interface remains the same,
> > independent of if a copper PHY is used, or a SFP. But an SFP does not
> > have an instance of a phydv.
>
> Well, as I said before in v1, CONFIG_NETWORK_PHY_TIMESTAMPING depends
> on phylib, plain and simple, and expanding beyond phylib is not within
> the scope of the this series.
True. But we also should be forward looking, to make sure we are not
heading into a dead end.
I'm currently thinking register_mii_timestamper() should take a netdev
argument, and the net_device structure should gain a struct
mii_timestamper.
But we have to look at the lifetime problems. A phydev does not know
what netdev it is associated to until phy_connect() is called. It is
at that point you can call register_mii_timestamper().
Andrew
^ permalink raw reply
* Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.
From: Richard Cochran @ 2018-10-07 21:07 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, devicetree, David Miller, Florian Fainelli, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007205906.3h7fpyxdhc24o6lc@localhost>
On Sun, Oct 07, 2018 at 01:59:06PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 09:54:00PM +0200, Andrew Lunn wrote:
> > 1) phylink, not phdev. We have been pushing some MAC drivers towards
> > phylink, especially those which support >1Gbp.
>
> If a phylink device appears that wants time stamping, can't we add the
> call to register_mii_timestamper()?
Actually, I see that 'struct phylink' has a 'struct phy_device *phydev',
and so it can implement the 'struct mii_timestamper' interface directly.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.
From: Richard Cochran @ 2018-10-07 20:59 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, devicetree, David Miller, Florian Fainelli, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007195400.GA25883@lunn.ch>
On Sun, Oct 07, 2018 at 09:54:00PM +0200, Andrew Lunn wrote:
> Sure, but things have moved on since then.
If you have a specific suggestion on how to better implement this,
please tell us what it is.
> I can think of three obvious use cases where this does not work:
>
> 1) phylink, not phdev. We have been pushing some MAC drivers towards
> phylink, especially those which support >1Gbp.
If a phylink device appears that wants time stamping, can't we add the
call to register_mii_timestamper()?
> 2) When an SFP is connected to the MAC, not a copper PHY. The class of
> device you are adding a driver for will work just as well for an SFP
> as for a copper PHY. The SERDES interface remains the same,
> independent of if a copper PHY is used, or a SFP. But an SFP does not
> have an instance of a phydv.
Well, as I said before in v1, CONFIG_NETWORK_PHY_TIMESTAMPING depends
on phylib, plain and simple, and expanding beyond phylib is not within
the scope of the this series.
> 3) Firmware controlled PHYs. phylib/phylink is not used, the MAC turns
> all ethtool calls into RPCs to the firmware. I've no numbers about
> this, but i have the feeling this is becoming more popular. It does
> however tend to be high end devices, and those are more likely to have
> timestamping in the MAC. I suppose they could also offload
> tomestamping to the firmware, in which case, they might want to make
> use of this new API.
Any MAC with private PHY stuff (that doesn't use phylib) can implement
SO_TIMESTAMPING directly, as if it were a MAC.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH net v2 0/2] net/smc: userspace breakage fixes
From: David Miller @ 2018-10-08 4:06 UTC (permalink / raw)
To: esyr; +Cc: netdev, linux-kernel, ubraun, kgraul, hwippel, sergei.shtylyov
In-Reply-To: <20181007145726.GA16173@asgard.redhat.com>
From: Eugene Syromiatnikov <esyr@redhat.com>
Date: Sun, 7 Oct 2018 16:57:26 +0200
> These two patches correct some userspace-affecting issues introduced
> during 4.19 development cycle, specifically:
> * New structure "struct smcd_diag_dmbinfo" has been defined in a way
> that would lead to different layout of the structure on most 32-bit
> ABIs in comparison with layout on 64-bit ABIs;
> * One of the commits renamed an UAPI-exposed field name.
>
> Changes since v1:
> * Managed not to forget to add --cover-letter.
> * Commit ID format in commit message has been changed in accordance
> with Sergei Shtylyov's recommendations.
Series applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH] net: sched: pie: fix coding style issues
From: David Miller @ 2018-10-08 3:39 UTC (permalink / raw)
To: lesliemonis; +Cc: jhs, netdev, linux-kernel
In-Reply-To: <1538855565-22986-1-git-send-email-lesliemonis@gmail.com>
From: Leslie Monis <lesliemonis@gmail.com>
Date: Sun, 7 Oct 2018 01:22:45 +0530
> Fix 5 warnings and 14 checks issued by checkpatch.pl:
...
> Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] bnxt_en: Remove unnecessary unsigned integer comparison and initialize variable
From: David Miller @ 2018-10-08 3:37 UTC (permalink / raw)
To: gustavo; +Cc: michael.chan, netdev, linux-kernel
In-Reply-To: <20181005201209.GA16515@embeddedor.com>
From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Date: Fri, 5 Oct 2018 22:12:09 +0200
> There is no need to compare *val.vu32* with < 0 because
> such variable is of type u32 (32 bits, unsigned), making it
> impossible to hold a negative value. Fix this by removing
> such comparison.
>
> Also, initialize variable *max_val* to -1, just in case
> it is not initialized to either BNXT_MSIX_VEC_MAX or
> BNXT_MSIX_VEC_MIN_MAX before using it in a comparison
> with val.vu32 at line 159:
>
> if (val.vu32 > max_val)
>
> Addresses-Coverity-ID: 1473915 ("Unsigned compared against 0")
> Addresses-Coverity-ID: 1473920 ("Uninitialized scalar variable")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Applied.
^ permalink raw reply
* Re: [PATCH] udp: Unbreak modules that rely on external __skb_recv_udp() availability
From: David Miller @ 2018-10-08 3:34 UTC (permalink / raw)
To: jikos; +Cc: yoshfuji, pabeni, edumazet, netdev, linux-kernel
In-Reply-To: <nycvar.YFH.7.76.1810041335180.14430@cbobk.fhfr.pm>
From: Jiri Kosina <jikos@kernel.org>
Date: Thu, 4 Oct 2018 13:37:32 +0200 (CEST)
> From: Jiri Kosina <jkosina@suse.cz>
>
> Commit 2276f58ac589 ("udp: use a separate rx queue for packet reception")
> turned static inline __skb_recv_udp() from being a trivial helper around
> __skb_recv_datagram() into a UDP specific implementaion, making it
> EXPORT_SYMBOL_GPL() at the same time.
>
> There are external modules that got broken by __skb_recv_udp() not being
> visible to them. Let's unbreak them by making __skb_recv_udp EXPORT_SYMBOL().
>
> Rationale (one of those) why this is actually "technically correct" thing
> to do: __skb_recv_udp() used to be an inline wrapper around
> __skb_recv_datagram(), which itself (still, and correctly so, I believe)
> is EXPORT_SYMBOL().
>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Fixes: 2276f58ac589 ("udp: use a separate rx queue for packet reception")
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Applied...
But waiting from 4.13 until now to bring this up is really pushing it...
^ permalink raw reply
* [PATCH] bpf: btf: Fix a missing check bug
From: Wenwen Wang @ 2018-10-07 20:23 UTC (permalink / raw)
To: Wenwen Wang
Cc: Kangjie Lu, Alexei Starovoitov, Daniel Borkmann,
open list:BPF (Safe dynamic programs and tools),
open list:BPF (Safe dynamic programs and tools)
In btf_parse_hdr(), the length of the btf data header is firstly copied
from the user space to 'hdr_len' and checked to see whether it is larger
than 'btf_data_size'. If yes, an error code EINVAL is returned. Otherwise,
the whole header is copied again from the user space to 'btf->hdr'.
However, after the second copy, there is no check between
'btf->hdr->hdr_len' and 'hdr_len' to confirm that the two copies get the
same value. Given that the btf data is in the user space, a malicious user
can race to change the data between the two copies. By doing so, the user
can provide malicious data to the kernel and cause undefined behavior.
This patch adds a necessary check after the second copy, to make sure
'btf->hdr->hdr_len' has the same value as 'hdr_len'. Otherwise, an error
code EINVAL will be returned.
Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
kernel/bpf/btf.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 2590700..7cce7db 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -2114,6 +2114,9 @@ static int btf_parse_hdr(struct btf_verifier_env *env, void __user *btf_data,
hdr = &btf->hdr;
+ if (hdr->hdr_len != hdr_len)
+ return -EINVAL;
+
btf_verifier_log_hdr(env, btf_data_size);
if (hdr->magic != BTF_MAGIC) {
--
2.7.4
^ permalink raw reply related
* RE: [v2, 1/5] net: dpaa2: move DPAA2 PTP driver out of staging/
From: Y.b. Lu @ 2018-10-08 3:11 UTC (permalink / raw)
To: Andrew Lunn, Ioana Ciocoi Radulescu
Cc: devel@driverdev.osuosl.org, netdev@vger.kernel.org,
Richard Cochran, linux-kernel@vger.kernel.org, Greg Kroah-Hartman,
David S . Miller
In-Reply-To: <20180929194616.GA11532@lunn.ch>
Hi Andrew,
Sorry for late. Just come back from holiday.
Hi Ioana,
Could you generate an additional patch for dpaa2_eth to address Andrew's comments?
There was also a warning when checked patch. FSL_DPAA2_ETH needed more description.
WARNING: please write a paragraph that describes the config symbol fully
#66: FILE: drivers/net/ethernet/freescale/dpaa2/Kconfig:1:
+config FSL_DPAA2_ETH
Thanks a lot.
Best regards,
Yangbo Lu
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Sunday, September 30, 2018 3:46 AM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org;
> netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>;
> David S . Miller <davem@davemloft.net>; Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Subject: Re: [v2, 1/5] net: dpaa2: move DPAA2 PTP driver out of staging/
>
> > +++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig
> > @@ -0,0 +1,15 @@
> > +config FSL_DPAA2_ETH
> > + tristate "Freescale DPAA2 Ethernet"
> > + depends on FSL_MC_BUS && FSL_MC_DPIO
>
> Could you add in here COMPILE_TEST?
>
> > + depends on NETDEVICES && ETHERNET
>
> With the move out of staging, i don't think these two are required.
>
> Andrew
^ permalink raw reply
* Re: [PATCH V2 net-next 4/5] net: mdio: of: Register discovered MII time stampers.
From: Andrew Lunn @ 2018-10-07 19:57 UTC (permalink / raw)
To: Richard Cochran
Cc: Florian Fainelli, netdev, devicetree, David Miller, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007192627.33nfcc27mvyqxbdy@localhost>
On Sun, Oct 07, 2018 at 12:26:27PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 08:17:54PM +0200, Andrew Lunn wrote:
> > > > + if (err == -ENOENT)
> > > > + return NULL;
> > > > + else if (err)
> > > > + return ERR_PTR(err);
> > > > +
> > > > + if (args.args_count >= 1)
> > > > + port = args.args[0];
> > >
> > > If it's greater than one, than it is an error, and it should be flagged
> > > as such.
> > >
> > > The idea looks good though, should of_find_mii_timestamper() somehow be
> > > made conditional to CONFIG_PTP and we should have a stub for when it is
> > > disabled?
> >
> > Hi Florian
> >
> > There already is a stub. But register return -EOPNOTSUPP.
>
> The stub returns NULL...
Ah, sorry, it is register_mii_tstamp_controller() which return
-EOPNOTSUP.
Andrew
^ permalink raw reply
* Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.
From: Andrew Lunn @ 2018-10-07 19:54 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, devicetree, David Miller, Florian Fainelli, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007191551.gy4l4g6qdgz6ztez@localhost>
On Sun, Oct 07, 2018 at 12:15:51PM -0700, Richard Cochran wrote:
> On Sun, Oct 07, 2018 at 08:27:51PM +0200, Andrew Lunn wrote:
> > The mii_timestamper is generic, in the same why hwmon is generic. It
> > does not matter where the time stamper is. So i'm wondering if we
> > should remove the special case for a PHY timestamper, remove all the
> > phylib support, etc.
>
> This implementation is (to the best of my understanding) what you were
> asking for in your review of v1:
Sure, but things have moved on since then.
> > So i really think you need to cleanly integrate into phylib and
> > phylink.
>
> > Use a phandle, and have
> > of_mdiobus_register_phy() follow the phandle to get the device.
>
> > To keep lifecycle issues simple, i would also keep it in phydev, not
> > netdev.
>
> This present series is a reasonable, incremental improvement to the
> existing PHY time stamping support. It will handle any use case that
> I can think of, and I would like to avoid over-engineering this.
I can think of three obvious use cases where this does not work:
1) phylink, not phdev. We have been pushing some MAC drivers towards
phylink, especially those which support >1Gbp.
2) When an SFP is connected to the MAC, not a copper PHY. The class of
device you are adding a driver for will work just as well for an SFP
as for a copper PHY. The SERDES interface remains the same,
independent of if a copper PHY is used, or a SFP. But an SFP does not
have an instance of a phydv.
2a) An SFP which is actually a Copper PHY. There is a phydev for this,
but it is associated to the phylink, not the netdev.
3) Firmware controlled PHYs. phylib/phylink is not used, the MAC turns
all ethtool calls into RPCs to the firmware. I've no numbers about
this, but i have the feeling this is becoming more popular. It does
however tend to be high end devices, and those are more likely to have
timestamping in the MAC. I suppose they could also offload
tomestamping to the firmware, in which case, they might want to make
use of this new API.
Andrew
^ permalink raw reply
* Re: [PATCH V2 net-next 0/5] Peer to Peer One-Step time stamping
From: Richard Cochran @ 2018-10-07 19:36 UTC (permalink / raw)
To: netdev
Cc: devicetree, Andrew Lunn, David Miller, Florian Fainelli,
Jacob Keller, Mark Rutland, Miroslav Lichvar, Rob Herring,
Willem de Bruijn
In-Reply-To: <20181007173823.21590-1-richardcochran@gmail.com>
On Sun, Oct 07, 2018 at 10:38:18AM -0700, Richard Cochran wrote:
> Changed in v2:
> ~~~~~~~~~~~~~~
> - Per the v1 review, changed the modeling of MII time stamping
> devices. They are no longer a kind of mdio device.
Forgot to add:
- Added method to callback into the driver after changes in link
status.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH V2 net-next 4/5] net: mdio: of: Register discovered MII time stampers.
From: Richard Cochran @ 2018-10-07 19:26 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, netdev, devicetree, David Miller, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007181754.GA22794@lunn.ch>
On Sun, Oct 07, 2018 at 08:17:54PM +0200, Andrew Lunn wrote:
> > > + if (err == -ENOENT)
> > > + return NULL;
> > > + else if (err)
> > > + return ERR_PTR(err);
> > > +
> > > + if (args.args_count >= 1)
> > > + port = args.args[0];
> >
> > If it's greater than one, than it is an error, and it should be flagged
> > as such.
> >
> > The idea looks good though, should of_find_mii_timestamper() somehow be
> > made conditional to CONFIG_PTP and we should have a stub for when it is
> > disabled?
>
> Hi Florian
>
> There already is a stub. But register return -EOPNOTSUPP.
The stub returns NULL...
> > > + return register_mii_timestamper(args.np, port);
>
> So this returns EOPNOTUP
NULL...
> > > static int of_mdiobus_register_phy(struct mii_bus *mdio,
> > > struct device_node *child, u32 addr)
> > > {
> > > + struct mii_timestamper *mii_ts;
> > > struct phy_device *phy;
> > > bool is_c45;
> > > int rc;
> > > u32 phy_id;
> > >
> > > + mii_ts = of_find_mii_timestamper(child);
> > > + if (IS_ERR(mii_ts))
> > > + return PTR_ERR(mii_ts);
> > > +
>
> and this returns EOPNOPTSUPP, so the PHY is not registered :-(
and the phydev.mii_ts field is then set to NULL.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH V2 net-next 4/5] net: mdio: of: Register discovered MII time stampers.
From: Richard Cochran @ 2018-10-07 19:23 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, devicetree, Andrew Lunn, David Miller, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <8a238abb-dfaa-54b4-1c10-86fa619f41e3@gmail.com>
On Sun, Oct 07, 2018 at 11:14:38AM -0700, Florian Fainelli wrote:
> There appears to be a binding document missing to describe what a
> timerstamper provider is. Using a more specific name than
> "#phandle-cells" is preferred when dealing with specific devices, e.g:
>
> interrupt-controller/#interrupt-cells
> clocks/#clock-cells
Sure.
> So I would go with #timestamp-cells here, and define what the cell sie
> and format should be in a separate "dt-bindings" prefixed patch that the
> Device Tree folks can also comment on.
I documented this in the last patch. I didn't see any example in our
device tree that explains a "reference" like this that is not
connected to a specific node type.
>
> > + if (err == -ENOENT)
> > + return NULL;
> > + else if (err)
> > + return ERR_PTR(err);
> > +
> > + if (args.args_count >= 1)
> > + port = args.args[0];
>
> If it's greater than one, than it is an error, and it should be flagged
> as such.
I wanted to allow specific MII time stamping drivers to use one than
one value in the future, should the need arise.
> The idea looks good though, should of_find_mii_timestamper() somehow be
> made conditional to CONFIG_PTP and we should have a stub for when it is
> disabled?
Do you mean CONFIG_NETWORK_PHY_TIMESTAMPING ?
There is a stub for that.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.
From: Richard Cochran @ 2018-10-07 19:15 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, devicetree, David Miller, Florian Fainelli, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007182751.GC22794@lunn.ch>
On Sun, Oct 07, 2018 at 08:27:51PM +0200, Andrew Lunn wrote:
> The mii_timestamper is generic, in the same why hwmon is generic. It
> does not matter where the time stamper is. So i'm wondering if we
> should remove the special case for a PHY timestamper, remove all the
> phylib support, etc.
This implementation is (to the best of my understanding) what you were
asking for in your review of v1:
> So i really think you need to cleanly integrate into phylib and
> phylink.
> Use a phandle, and have
> of_mdiobus_register_phy() follow the phandle to get the device.
> To keep lifecycle issues simple, i would also keep it in phydev, not
> netdev.
This present series is a reasonable, incremental improvement to the
existing PHY time stamping support. It will handle any use case that
I can think of, and I would like to avoid over-engineering this.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH rdma-next 3/4] IB/mlx5: Verify that driver supports user flags
From: Jason Gunthorpe @ 2018-10-07 19:13 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Guy Levi,
Yonatan Cohen, Saeed Mahameed, linux-netdev
In-Reply-To: <20181007090337.21037-4-leon@kernel.org>
On Sun, Oct 07, 2018 at 12:03:36PM +0300, Leon Romanovsky wrote:
> From: Yonatan Cohen <yonatanc@mellanox.com>
>
> Flags sent down from user might not be supported by
> running driver.
> This might lead to unwanted bugs.
> To solve this, added macro to test for unsupported flags.
>
> Signed-off-by: Yonatan Cohen <yonatanc@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/hw/mlx5/qp.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> index bae48bdf281c..17c4b6641933 100644
> +++ b/drivers/infiniband/hw/mlx5/qp.c
> @@ -1728,6 +1728,15 @@ static void configure_requester_scat_cqe(struct mlx5_ib_dev *dev,
> MLX5_SET(qpc, qpc, cs_req, MLX5_REQ_SCAT_DATA32_CQE);
> }
>
> +#define MLX5_QP_CREATE_FLAGS_NOT_SUPPORTED(flags) \
> + ((flags) & ~( \
This needs a cast, it would be better to add something like the check
comp mask function in rdma-core than this goofy macro thing.
Jason
^ permalink raw reply
* Re: [PATCH net V2] vhost-vsock: fix use after free
From: Jason Wang @ 2018-10-08 2:20 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, sergei.shtylyov, netdev, linux-kernel, virtualization,
stefanha
In-Reply-To: <20180927194734-mutt-send-email-mst@kernel.org>
On 2018年09月28日 07:50, Michael S. Tsirkin wrote:
> On Fri, Sep 28, 2018 at 07:37:37AM +0800, Jason Wang wrote:
>>
>> On 2018年09月28日 01:04, Michael S. Tsirkin wrote:
>>> On Thu, Sep 27, 2018 at 08:22:04PM +0800, Jason Wang wrote:
>>>> The access of vsock is not protected by vhost_vsock_lock. This may
>>>> lead to use after free since vhost_vsock_dev_release() may free the
>>>> pointer at the same time.
>>>>
>>>> Fix this by holding the lock during the access.
>>>>
>>>> Reported-by:syzbot+e3e074963495f92a89ed@syzkaller.appspotmail.com
>>>> Fixes: 16320f363ae1 ("vhost-vsock: add pkt cancel capability")
>>>> Fixes: 433fc58e6bf2 ("VSOCK: Introduce vhost_vsock.ko")
>>>> Cc: Stefan Hajnoczi<stefanha@redhat.com>
>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>> Wow is that really the best we can do?
>> For net/stable, probably yes.
>>
>>> A global lock on a data path
>>> operation?
>> It's already there,
> &vhost_vsock_lock? were is it takes on data path?
Ok, but the current code use list which means a global lock is needed
anyway here.
>
>> and the patch only increase the critical section.
>>
>>> Granted use after free is nasty but Stefan said he sees
>>> a way to fix it using a per socket refcount. He's on vacation
>>> until Oct 4 though ...
>>>
>> Stefan has acked the pacth, so I think it's ok? We can do optimization for
>> -next on top.
>>
>> Thanks
>
> Well on high SMP serializing can drop performance as much as x100 so I'm
> not sure it's appropriate - seems to fix a bug but can introduce a
> regression. Let's see how does a proper fix look first?
>
It looks to me hlist + RCU is better. But I'm not sure it's suitable for
-net/-stable.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.
From: Florian Fainelli @ 2018-10-07 19:06 UTC (permalink / raw)
To: Andrew Lunn, Richard Cochran
Cc: netdev, devicetree, David Miller, Jacob Keller, Mark Rutland,
Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007182751.GC22794@lunn.ch>
On 10/7/2018 11:27 AM, Andrew Lunn wrote:
> On Sun, Oct 07, 2018 at 10:38:20AM -0700, Richard Cochran wrote:
>> Currently the stack supports time stamping in PHY devices. However,
>> there are newer, non-PHY devices that can snoop an MII bus and provide
>> time stamps. In order to support such devices, this patch introduces
>> a new interface to be used by both PHY and non-PHY devices.
>>
>> In addition, the one and only user of the old PHY time stamping API is
>> converted to the new interface.
>
> Hi Richard
>
> I'm a bit undecided about this. If you look at how we do HWMON sensors
> in PHYs, the probe function just registers with the HWMON subsystem.
> We don't have any support in phy_device, or anywhere else in the PHY
> core.
>
> The mii_timestamper is generic, in the same why hwmon is generic. It
> does not matter where the time stamper is. So i'm wondering if we
> should remove the special case for a PHY timestamper, remove all the
> phylib support, etc.
>
> I need to look at the other patches and see how this all fits
> together.
Agreed, the fact that some PHYs capable of timestamping and register
themselves as a timestamper makes sense, whether this needs to be backed
into the core PHYLIB might have been something convenient at some point,
but maybe we can revisit that paradigm now that there is more generic
timestamping provider framework being proposed here.
--
Florian
^ permalink raw reply
* Re: [PATCH V2 net-next 2/5] net: Introduce a new MII time stamping interface.
From: Andrew Lunn @ 2018-10-07 18:27 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, devicetree, David Miller, Florian Fainelli, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007173823.21590-3-richardcochran@gmail.com>
On Sun, Oct 07, 2018 at 10:38:20AM -0700, Richard Cochran wrote:
> Currently the stack supports time stamping in PHY devices. However,
> there are newer, non-PHY devices that can snoop an MII bus and provide
> time stamps. In order to support such devices, this patch introduces
> a new interface to be used by both PHY and non-PHY devices.
>
> In addition, the one and only user of the old PHY time stamping API is
> converted to the new interface.
Hi Richard
I'm a bit undecided about this. If you look at how we do HWMON sensors
in PHYs, the probe function just registers with the HWMON subsystem.
We don't have any support in phy_device, or anywhere else in the PHY
core.
The mii_timestamper is generic, in the same why hwmon is generic. It
does not matter where the time stamper is. So i'm wondering if we
should remove the special case for a PHY timestamper, remove all the
phylib support, etc.
I need to look at the other patches and see how this all fits
together.
Andrew
^ permalink raw reply
* Re: [PATCH V2 net-next 4/5] net: mdio: of: Register discovered MII time stampers.
From: Andrew Lunn @ 2018-10-07 18:19 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, devicetree, David Miller, Florian Fainelli, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007173823.21590-5-richardcochran@gmail.com>
On Sun, Oct 07, 2018 at 10:38:22AM -0700, Richard Cochran wrote:
> When parsing a PHY node, register its time stamper, if any, and attach
> the instance to the PHY device.
Hi Richard
This does look a lot better.
Thanks for making the changes.
Andrew
^ permalink raw reply
* Re: [PATCH V2 net-next 4/5] net: mdio: of: Register discovered MII time stampers.
From: Andrew Lunn @ 2018-10-07 18:17 UTC (permalink / raw)
To: Florian Fainelli
Cc: Richard Cochran, netdev, devicetree, David Miller, Jacob Keller,
Mark Rutland, Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <8a238abb-dfaa-54b4-1c10-86fa619f41e3@gmail.com>
> > + if (err == -ENOENT)
> > + return NULL;
> > + else if (err)
> > + return ERR_PTR(err);
> > +
> > + if (args.args_count >= 1)
> > + port = args.args[0];
>
> If it's greater than one, than it is an error, and it should be flagged
> as such.
>
> The idea looks good though, should of_find_mii_timestamper() somehow be
> made conditional to CONFIG_PTP and we should have a stub for when it is
> disabled?
Hi Florian
There already is a stub. But register return -EOPNOTSUPP.
> > +
> > + return register_mii_timestamper(args.np, port);
So this returns EOPNOTUP
> > +}
> > +
> > static int of_mdiobus_register_phy(struct mii_bus *mdio,
> > struct device_node *child, u32 addr)
> > {
> > + struct mii_timestamper *mii_ts;
> > struct phy_device *phy;
> > bool is_c45;
> > int rc;
> > u32 phy_id;
> >
> > + mii_ts = of_find_mii_timestamper(child);
> > + if (IS_ERR(mii_ts))
> > + return PTR_ERR(mii_ts);
> > +
and this returns EOPNOPTSUPP, so the PHY is not registered :-(
Andrew
^ permalink raw reply
* Re: [PATCH V2 net-next 4/5] net: mdio: of: Register discovered MII time stampers.
From: Florian Fainelli @ 2018-10-07 18:14 UTC (permalink / raw)
To: Richard Cochran, netdev
Cc: devicetree, Andrew Lunn, David Miller, Jacob Keller, Mark Rutland,
Miroslav Lichvar, Rob Herring, Willem de Bruijn
In-Reply-To: <20181007173823.21590-5-richardcochran@gmail.com>
On 10/07/18 10:38, Richard Cochran wrote:
> When parsing a PHY node, register its time stamper, if any, and attach
> the instance to the PHY device.
>
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
> drivers/net/phy/phy_device.c | 3 +++
> drivers/of/of_mdio.c | 26 ++++++++++++++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index a454432d166f..c24bce9b7270 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -833,6 +833,9 @@ EXPORT_SYMBOL(phy_device_register);
> */
> void phy_device_remove(struct phy_device *phydev)
> {
> + if (phydev->mii_ts)
> + unregister_mii_timestamper(phydev->mii_ts);
> +
> device_del(&phydev->mdio.dev);
>
> /* Assert the reset signal */
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index f76c10ecc616..7699f167e4a9 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -44,14 +44,38 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id)
> return -EINVAL;
> }
>
> +struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
> +{
> + struct of_phandle_args args;
> + unsigned int port = 0;
> + int err;
> +
> + err = of_parse_phandle_with_args(node, "timestamper",
> + "#phandle-cells", 0, &args);
There appears to be a binding document missing to describe what a
timerstamper provider is. Using a more specific name than
"#phandle-cells" is preferred when dealing with specific devices, e.g:
interrupt-controller/#interrupt-cells
clocks/#clock-cells
etc.
So I would go with #timestamp-cells here, and define what the cell sie
and format should be in a separate "dt-bindings" prefixed patch that the
Device Tree folks can also comment on.
> + if (err == -ENOENT)
> + return NULL;
> + else if (err)
> + return ERR_PTR(err);
> +
> + if (args.args_count >= 1)
> + port = args.args[0];
If it's greater than one, than it is an error, and it should be flagged
as such.
The idea looks good though, should of_find_mii_timestamper() somehow be
made conditional to CONFIG_PTP and we should have a stub for when it is
disabled?
> +
> + return register_mii_timestamper(args.np, port);
> +}
> +
> static int of_mdiobus_register_phy(struct mii_bus *mdio,
> struct device_node *child, u32 addr)
> {
> + struct mii_timestamper *mii_ts;
> struct phy_device *phy;
> bool is_c45;
> int rc;
> u32 phy_id;
>
> + mii_ts = of_find_mii_timestamper(child);
> + if (IS_ERR(mii_ts))
> + return PTR_ERR(mii_ts);
> +
> is_c45 = of_device_is_compatible(child,
> "ethernet-phy-ieee802.3-c45");
>
> @@ -97,6 +121,8 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
> return rc;
> }
>
> + phy->mii_ts = mii_ts;
> +
> dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> child->name, addr);
> return 0;
>
--
Florian
^ permalink raw reply
* Re: [PATCH v2 2/2] netdev/phy: add MDIO bus multiplexer driven by a regmap
From: Florian Fainelli @ 2018-10-07 18:08 UTC (permalink / raw)
To: Pankaj Bansal, Andrew Lunn; +Cc: netdev
In-Reply-To: <20181007182423.7342-3-pankaj.bansal@nxp.com>
On 10/07/18 11:24, Pankaj Bansal wrote:
> Add support for an MDIO bus multiplexer controlled by a regmap
> device, like an FPGA.
>
> Tested on a NXP LX2160AQDS board which uses the "QIXIS" FPGA
> attached to the i2c bus.
>
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>
> Notes:
> V2:
> - replaced be32_to_cpup with of_property_read_u32
> - incorporated Andrew's comments
>
> drivers/net/phy/Kconfig | 13 +++
> drivers/net/phy/Makefile | 1 +
> drivers/net/phy/mdio-mux-regmap.c | 171 ++++++++++++++++++++++++++++
> 3 files changed, 185 insertions(+)
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 82070792edbb..d1ac9e70cbb2 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -87,6 +87,19 @@ config MDIO_BUS_MUX_MMIOREG
>
> Currently, only 8/16/32 bits registers are supported.
>
> +config MDIO_BUS_MUX_REGMAP
> + tristate "REGMAP controlled MDIO bus multiplexers"
> + depends on OF_MDIO && REGMAP
> + select MDIO_BUS_MUX
> + help
> + This module provides a driver for MDIO bus multiplexers that
> + are controlled via a regmap device, like an FPGA connected to i2c.
> + The multiplexer connects one of several child MDIO busses to a
> + parent bus.Child bus selection is under the control of one of
> + the FPGA's registers.
> +
> + Currently, only upto 32 bits registers are supported.
> +
> config MDIO_CAVIUM
> tristate
>
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 5805c0b7d60e..33053f9f320d 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_MDIO_BUS_MUX) += mdio-mux.o
> obj-$(CONFIG_MDIO_BUS_MUX_BCM_IPROC) += mdio-mux-bcm-iproc.o
> obj-$(CONFIG_MDIO_BUS_MUX_GPIO) += mdio-mux-gpio.o
> obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) += mdio-mux-mmioreg.o
> +obj-$(CONFIG_MDIO_BUS_MUX_REGMAP) += mdio-mux-regmap.o
> obj-$(CONFIG_MDIO_CAVIUM) += mdio-cavium.o
> obj-$(CONFIG_MDIO_GPIO) += mdio-gpio.o
> obj-$(CONFIG_MDIO_HISI_FEMAC) += mdio-hisi-femac.o
> diff --git a/drivers/net/phy/mdio-mux-regmap.c b/drivers/net/phy/mdio-mux-regmap.c
> new file mode 100644
> index 000000000000..6068d05a728a
> --- /dev/null
> +++ b/drivers/net/phy/mdio-mux-regmap.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/* Simple regmap based MDIO MUX driver
> + *
> + * Copyright 2018 NXP
> + *
> + * Based on mdio-mux-mmioreg.c by Timur Tabi
> + *
> + * Author:
> + * Pankaj Bansal <pankaj.bansal@nxp.com>
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/device.h>
> +#include <linux/of_mdio.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +#include <linux/mdio-mux.h>
> +#include <linux/regmap.h>
> +
> +struct mdio_mux_regmap_state {
> + void *mux_handle;
> + struct regmap *regmap;
> + u32 mux_reg;
> + u32 mask;
> +};
> +
> +/* MDIO multiplexing switch function
> + *
> + * This function is called by the mdio-mux layer when it thinks the mdio bus
> + * multiplexer needs to switch.
> + *
> + * 'current_child' is the current value of the mux register (masked via
> + * s->mask).
> + *
> + * 'desired_child' is the value of the 'reg' property of the target child MDIO
> + * node.
> + *
> + * The first time this function is called, current_child == -1.
> + *
> + * If current_child == desired_child, then the mux is already set to the
> + * correct bus.
> + */
> +static int mdio_mux_regmap_switch_fn(int current_child, int desired_child,
> + void *data)
> +{
> + struct mdio_mux_regmap_state *s = data;
> + bool change;
> + int ret;
> +
> + ret = regmap_update_bits_check(s->regmap,
> + s->mux_reg,
> + s->mask,
> + desired_child,
> + &change);
> +
> + if (ret)
> + return ret;
> + if (change)
> + pr_debug("%s %d -> %d\n", __func__, current_child,
> + desired_child);
If you add a struct platform_device or struct device reference to struct
mdio_mux_regmap_state, the you can use dev_dbg() here with the correct
device, which would be helpful if you are debugging problems, and there
are more than once instance of them in the system.
> + return ret;
> +}
> +
> +static int mdio_mux_regmap_probe(struct platform_device *pdev)
> +{
> + struct device_node *np2, *np = pdev->dev.of_node;
How about naming "np2", "child" instead?
Everything else looks fine to me, thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer driven by a regmap device
From: Florian Fainelli @ 2018-10-07 18:01 UTC (permalink / raw)
To: Pankaj Bansal, Andrew Lunn; +Cc: netdev
In-Reply-To: <20181007182423.7342-2-pankaj.bansal@nxp.com>
On 10/07/18 11:24, Pankaj Bansal wrote:
> Add support for an MDIO bus multiplexer controlled by a regmap
> device, like an FPGA.
>
> Tested on a NXP LX2160AQDS board which uses the "QIXIS" FPGA
> attached to the i2c bus.
>
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>
> Notes:
> V2:
> - Fixed formatting error caused by using space instead of tab
> - Add newline between property list and subnode
> - Add newline between two subnodes
>
> .../bindings/net/mdio-mux-regmap.txt | 95 ++++++++++++++++++
> 1 file changed, 95 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> new file mode 100644
> index 000000000000..88ebac26c1c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> @@ -0,0 +1,95 @@
> +Properties for an MDIO bus multiplexer controlled by a regmap
> +
> +This is a special case of a MDIO bus multiplexer. A regmap device,
> +like an FPGA, is used to control which child bus is connected. The mdio-mux
> +node must be a child of the device that is controlled by a regmap.
> +The driver currently only supports devices with upto 32-bit registers.
I would omit any sort of details about Linux constructs designed to
support specific needs (e.g: regmap) as well as putting driver
limitations into a binding document because it really ought to be
restricted to describing hardware.
> +
> +Required properties in addition to the generic multiplexer properties:
> +
> +- compatible : string, must contain "mdio-mux-regmap"
> +
> +- reg : integer, contains the offset of the register that controls the bus
> + multiplexer. it can be 32 bit number.
Technically it could be any "reg" property size, the only requirement is
that it must be of the appropriate size with respect to what the parent
node of this "mdio-mux-regmap" node has, determined by
#address-cells/#size-cells width.
> +
> +- mux-mask : integer, contains an 32 bit mask that specifies which
> + bits in the register control the actual bus multiplexer. The
> + 'reg' property of each child mdio-mux node must be constrained by
> + this mask.
Same thing here.
Since this is a MDIO mux, I would invite you to specify what the correct
#address-cells/#size-cells values should be (1, and 0 respectively as
your example correctly shows).
> +
> +Example:
> +
> +The FPGA node defines a i2c connected FPGA with a register space of 0x30 bytes.
> +For the "EMI2" MDIO bus, register 0x54 (BRDCFG4) controls the mux on that bus.
> +A bitmask of 0x07 means that bits 0, 1 and 2 (bit 0 is lsb) are the bits on
> +BRDCFG4 that control the actual mux.
> +
> +i2c@2000000 {
> + compatible = "fsl,vf610-i2c";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x0 0x2000000 0x0 0x10000>;
> + interrupts = <0 34 0x4>; // Level high type
> + clock-names = "i2c";
> + clocks = <&clockgen 4 7>;
> + fsl-scl-gpio = <&gpio2 15 0>;
> + status = "okay";
> +
> + /* The FPGA node */
> + fpga@66 {
> + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
> + reg = <0x66>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mdio1_mux@54 {
> + compatible = "mdio-mux-regmap", "mdio-mux";
> + mdio-parent-bus = <&emdio2>; /* MDIO bus */
> + reg = <0x54>; /* BRDCFG4 */
> + mux-mask = <0x07>; /* EMI2_MDIO */
> + #address-cells=<1>;
> + #size-cells = <0>;
> +
> + mdio1_ioslot1@0 { // Slot 1
> + reg = <0x00>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + phy1@1 {
> + reg = <1>;
> + compatible = "ethernet-phy-id0210.7441";
> + };
> +
> + phy1@0 {
> + reg = <0>;
> + compatible = "ethernet-phy-id0210.7441";
> + };
> + };
> +
> + mdio1_ioslot2@1 { // Slot 2
> + reg = <0x01>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + };
> +
> + mdio1_ioslot3@2 { // Slot 3
> + reg = <0x02>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + };
> + };
> + };
> +};
> +
> + /* The parent MDIO bus. */
> + emdio2: mdio@0x8B97000 {
> + compatible = "fsl,fman-memac-mdio";
> + reg = <0x0 0x8B97000 0x0 0x1000>;
> + device_type = "mdio";
> + little-endian;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
>
--
Florian
^ permalink raw reply
* Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
From: Jason A. Donenfeld @ 2018-10-08 1:06 UTC (permalink / raw)
To: esyr; +Cc: LKML, Netdev, David Miller, Greg Kroah-Hartman
In-Reply-To: <20181006194348.GK32759@asgard.redhat.com>
Hey Eugene,
On Sat, Oct 6, 2018 at 9:43 PM Eugene Syromiatnikov <esyr@redhat.com> wrote:
>
> On Sat, Oct 06, 2018 at 04:57:09AM +0200, Jason A. Donenfeld wrote:
> > +static int get_allowedips(void *ctx, const u8 *ip, u8 cidr, int family)
> > +{
> > + struct allowedips_ctx *actx = ctx;
> > + struct nlattr *allowedip_nest;
> > +
> > + allowedip_nest = nla_nest_start(actx->skb, actx->i++);
>
> Second parameter of nl_nest_start is an attribute type; (ab)using it as
> array index leads to special handling of such structures in parsers.
> It's better to have some type like WGDEVICE_A_PEER_ITEM and provide an
> additional attribute inside it for index (WGPEER_A_INDEX?).
> See, for example, commit v4.12-rc1~119^2~131 ("nbd: add a status netlink
> command").
>
> > +static int get_peer(struct wireguard_peer *peer, unsigned int index,
> > + struct allowedips_cursor *rt_cursor, struct sk_buff *skb)
> > +{
> > + struct nlattr *allowedips_nest, *peer_nest = nla_nest_start(skb, index);
>
> Same here.
Good point. Actually the index aspect is totally arbitrary and not
useful at all, and so that part can just be entirely excised.
Jason
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox