Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net-next 0/2] net: dsa: cross-chip FDB support
From: David Miller @ 2017-12-03  2:22 UTC (permalink / raw)
  To: vivien.didelot; +Cc: netdev, linux-kernel, kernel, f.fainelli, andrew
In-Reply-To: <20171130175643.14022-1-vivien.didelot@savoirfairelinux.com>

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Thu, 30 Nov 2017 12:56:41 -0500

> DSA can have interconnected switches. For instance, the ZII Dev Rev B
> board described in arch/arm/boot/dts/vf610-zii-dev-rev-b.dts has a
> switch fabric composed of 3 switch devices like this:
> 
>                           lan4                 lan6
>         CPU (eth1)            |  lan5         |  lan7
>                   |           | |             | |
>        [0 1 2 3 4 6 5]---[6 0 1 2 3 4 5]---[9 0 1 2 3 4 5 6 7 8]
>         | | |               |                     | | |
>     lan0  |  lan2       lan3                  lan8  |  optical4
>            lan1                                      optical3
> 
> One current issue with DSA is cross-chip FDB. If we add a static MAC
> address on lan3, only its parent switch 1 (the one in the middle) will
> be programmed. That is not correct in a cross-chip environment, because
> the DSA ports connecting to switch 1 of adjacent switch 0 (on the left)
> and switch 2 (on the right) must be programmed too.
> 
> Without this patchset, a dump of the hardware FDB of switches 0, 1 and 2
> after programming a MAC address on lan3 looks like this (*):
> 
>     # bridge fdb add 11:22:33:44:55:66 dev lan3
>     # cat /sys/kernel/debug/mv88e6xxx/sw*/atu/0 | grep -v FID
>        0  ff:ff:ff:ff:ff:ff            MC_STATIC       n  0 1 2 3 4 5 6
>        0  11:22:33:44:55:66    MC_STATIC_MGMT_PO       n  0 - - - - - -
>        0  ff:ff:ff:ff:ff:ff            MC_STATIC       n  0 1 2 3 4 5 6
>        0  ff:ff:ff:ff:ff:ff            MC_STATIC       n  0 1 2 3 4 5 6 7 8 9
> 
> With this patchset applied, adjacent DSA ports get programmed too:
> 
>     # bridge fdb add 11:22:33:44:55:66 dev lan3
>     # cat /sys/kernel/debug/mv88e6xxx/sw*/atu/0 | grep -v FID
>        0  11:22:33:44:55:66    MC_STATIC_MGMT_PO       n  - - - - - 5 -
>        0  ff:ff:ff:ff:ff:ff            MC_STATIC       n  0 1 2 3 4 5 6
>        0  11:22:33:44:55:66    MC_STATIC_MGMT_PO       n  0 - - - - - -
>        0  ff:ff:ff:ff:ff:ff            MC_STATIC       n  0 1 2 3 4 5 6
>        0  11:22:33:44:55:66    MC_STATIC_MGMT_PO       n  - - - - - - - - - 9
>        0  ff:ff:ff:ff:ff:ff            MC_STATIC       n  0 1 2 3 4 5 6 7 8 9
 ...

Series applied, thanks.

^ permalink raw reply

* Re: [PATCH net-next 0/4] net: dsa: simplify switchdev prepare phase
From: David Miller @ 2017-12-03  2:19 UTC (permalink / raw)
  To: vivien.didelot; +Cc: netdev, linux-kernel, kernel, f.fainelli, andrew
In-Reply-To: <20171130162400.8617-1-vivien.didelot@savoirfairelinux.com>

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Thu, 30 Nov 2017 11:23:56 -0500

> This patch series brings no functional changes.
> 
> It removes the unused switchdev_trans arguments from the dsa_switch_ops
> for both MDB and VLAN operations, and provides functions to prepare and
> add these objects for a given bitmap of ports.

Series applied.

^ permalink raw reply

* Re: [PATCH net-next] openvswitch: do not propagate headroom updates to internal port
From: David Miller @ 2017-12-03  2:17 UTC (permalink / raw)
  To: pabeni; +Cc: netdev, pshelar
In-Reply-To: <d8c2af0a398ed201064f39a348a55451bf34cd37.1512052527.git.pabeni@redhat.com>

From: Paolo Abeni <pabeni@redhat.com>
Date: Thu, 30 Nov 2017 15:35:33 +0100

> After commit 3a927bc7cf9d ("ovs: propagate per dp max headroom to
> all vports") the need_headroom for the internal vport is updated
> accordingly to the max needed headroom in its datapath.
> 
> That avoids the pskb_expand_head() costs when sending/forwarding
> packets towards tunnel devices, at least for some scenarios.
> 
> We still require such copy when using the ovs-preferred configuration
> for vxlan tunnels:
> 
>     br_int
>   /       \
> tap      vxlan
>            (remote_ip:X)
> 
> br_phy
>      \
>     NIC
> 
> where the route towards the IP 'X' is via 'br_phy'.
> 
> When forwarding traffic from the tap towards the vxlan device, we
> will call pskb_expand_head() in vxlan_build_skb() because
> br-phy->needed_headroom is equal to tun->needed_headroom.
> 
> With this change we avoid updating the internal vport needed_headroom,
> so that in the above scenario no head copy is needed, giving 5%
> performance improvement in UDP throughput test.
> 
> As a trade-off, packets sent from the internal port towards a tunnel
> device will now experience the head copy overhead. The rationale is
> that the latter use-case is less relevant performance-wise.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Applied, thanks.

^ permalink raw reply

* pull-request: bpf-next 2017-12-03
From: Daniel Borkmann @ 2017-12-03  1:00 UTC (permalink / raw)
  To: davem; +Cc: daniel, ast, netdev

Hi David,

The following pull-request contains BPF updates for your *net-next* tree.

The main changes are:

1) Addition of a software model for BPF offloads in order to ease
   testing code changes in that area and make semantics more clear.
   This is implemented in a new driver called netdevsim, which can
   later also be extended for other offloads. SR-IOV support is added
   as well to netdevsim. BPF kernel selftests for offloading are
   added so we can track basic functionality as well as exercising
   all corner cases around BPF offloading, from Jakub.

2) Today drivers have to drop the reference on BPF progs they hold
   due to XDP on device teardown themselves. Change this in order
   to make XDP handling inside the drivers less error prone, and
   move disabling XDP to the core instead, also from Jakub.

3) Misc set of BPF verifier improvements and cleanups as preparatory
   work for upcoming BPF-to-BPF calls. Among others, this set also
   improves liveness marking such that pruning can be slightly more
   effective. Register and stack liveness information is now included
   in the verifier log as well, from Alexei.

4) nfp JIT improvements in order to identify load/store sequences in
   the BPF prog e.g. coming from memcpy lowering and optimizing them
   through the NPU's command push pull (CPP) instruction, from Jiong.

5) Cleanups to test_cgrp2_attach2.c BPF sample code in oder to remove
   bpf_prog_attach() magic values and replacing them with actual proper
   attach flag instead, from David.

Please consider pulling these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git

Thanks a lot!

----------------------------------------------------------------

The following changes since commit 9f66816a6a4dd740bfa29cc8a8e19b90fd7df4e7:

  net: dsa: bcm_sf2: Utilize b53_get_tag_protocol() (2017-11-30 13:00:04 -0500)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git 

for you to fetch changes up to 6720f1084c066a5ba051a250e9d5d8c2ad4f554c:

  Merge branch 'bpf-xdp-stack-uninit-and-offload-tests' (2017-12-03 00:27:59 +0100)

----------------------------------------------------------------
Alexei Starovoitov (7):
      bpf: fix stack state printing in verifier log
      bpf: print liveness info to verifier log
      bpf: don't mark FP reg as uninit
      bpf: improve verifier liveness marks
      bpf: improve JEQ/JNE path walking
      bpf: cleanup register_is_null()
      selftests/bpf: adjust test_align expected output

Daniel Borkmann (3):
      Merge branch 'bpf-verifier-misc-improvements'
      Merge branch 'bpf-nfp-jmp-memcpy-improvements'
      Merge branch 'bpf-xdp-stack-uninit-and-offload-tests'

David Ahern (1):
      samples/bpf: Convert magic numbers to names in multi-prog cgroup test case

Jakub Kicinski (10):
      nfp: fix old kdoc issues
      nfp: bpf: encode indirect commands
      net: xdp: avoid output parameters when querying XDP prog
      net: xdp: report flags program was installed with on query
      net: xdp: make the stack take care of the tear down
      netdevsim: add software driver for testing offloads
      netdevsim: add bpf offload support
      selftests/bpf: add offload test based on netdevsim
      netdevsim: add SR-IOV functionality
      net: dummy: remove fake SR-IOV functionality

Jiong Wang (11):
      nfp: bpf: support backward jump
      nfp: bpf: record jump destination to simplify jump fixup
      nfp: bpf: flag jump destination to guide insn combine optimizations
      nfp: bpf: don't do ld/mask combination if mask is jump destination
      nfp: bpf: don't do ld/shifts combination if shifts are jump destination
      nfp: bpf: relax source operands check
      nfp: bpf: correct the encoding for No-Dest immed
      nfp: bpf: factor out is_mbpf_load & is_mbpf_store
      nfp: bpf: implement memory bulk copy for length within 32-bytes
      nfp: bpf: implement memory bulk copy for length bigger than 32-bytes
      nfp: bpf: detect load/store sequences lowered from memory copy

 MAINTAINERS                                        |   5 +
 drivers/net/Kconfig                                |  11 +
 drivers/net/Makefile                               |   1 +
 drivers/net/dummy.c                                | 215 +------
 drivers/net/ethernet/broadcom/bnxt/bnxt.c          |   2 -
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c  |   3 -
 drivers/net/ethernet/netronome/nfp/bpf/jit.c       | 487 +++++++++++++--
 drivers/net/ethernet/netronome/nfp/bpf/main.c      |   7 -
 drivers/net/ethernet/netronome/nfp/bpf/main.h      |  35 +-
 drivers/net/ethernet/netronome/nfp/bpf/offload.c   |  23 +-
 drivers/net/ethernet/netronome/nfp/bpf/verifier.c  |   8 +-
 drivers/net/ethernet/netronome/nfp/nfp_asm.c       |   7 +-
 drivers/net/ethernet/netronome/nfp/nfp_asm.h       |   7 +-
 drivers/net/ethernet/netronome/nfp/nfp_net.h       |   2 +
 .../net/ethernet/netronome/nfp/nfp_net_common.c    |   4 +-
 .../ethernet/netronome/nfp/nfpcore/nfp_cppcore.c   |   9 +-
 drivers/net/ethernet/qlogic/qede/qede_main.c       |   4 -
 drivers/net/netdevsim/Makefile                     |   7 +
 drivers/net/netdevsim/bpf.c                        | 373 +++++++++++
 drivers/net/netdevsim/netdev.c                     | 502 +++++++++++++++
 drivers/net/netdevsim/netdevsim.h                  |  78 +++
 drivers/net/tun.c                                  |   4 -
 include/linux/netdevice.h                          |   5 +-
 kernel/bpf/verifier.c                              |  62 +-
 net/core/dev.c                                     |  53 +-
 net/core/rtnetlink.c                               |   6 +-
 samples/bpf/test_cgrp2_attach2.c                   |  36 +-
 tools/testing/selftests/bpf/Makefile               |   5 +-
 tools/testing/selftests/bpf/sample_ret0.c          |   7 +
 tools/testing/selftests/bpf/test_align.c           | 156 ++---
 tools/testing/selftests/bpf/test_offload.py        | 681 +++++++++++++++++++++
 31 files changed, 2362 insertions(+), 443 deletions(-)
 create mode 100644 drivers/net/netdevsim/Makefile
 create mode 100644 drivers/net/netdevsim/bpf.c
 create mode 100644 drivers/net/netdevsim/netdev.c
 create mode 100644 drivers/net/netdevsim/netdevsim.h
 create mode 100644 tools/testing/selftests/bpf/sample_ret0.c
 create mode 100755 tools/testing/selftests/bpf/test_offload.py

^ permalink raw reply

* Re: Fixing CVE-2017-16939 in v4.4.y and possibly v3.18.y
From: Michal Kubecek @ 2017-12-03  0:44 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: netdev, stable, Greg Kroah-Hartman
In-Reply-To: <4ac5fb52-28f9-7111-483d-f010b093d923@roeck-us.net>

On Sat, Dec 02, 2017 at 04:20:40PM -0800, Guenter Roeck wrote:
> On 12/01/2017 11:48 AM, Michal Kubecek wrote:
> > On Thu, Nov 30, 2017 at 10:37:40AM -0800, Guenter Roeck wrote:
> > > Hi,
> > > 
> > > The fix for CVE-2017-16939 has been applied to v4.9.y, but not to v4.4.y
> > > and older kernels. However, I confirmed that running the published POC
> > > (see https://blogs.securiteam.com/index.php/archives/3535) does crash a 4.4
> > > kernel.
> > > 
> > > I confirmed that the following two patches fix the problem in v4.4.y.
> > > Please consider applying them to v4.4.y (and possibly v3.18.y).
> > > 
> > > fc9e50f5a5a4e ("netlink: add a start callback for starting a netlink dump")
> > > 1137b5e2529a8 ("ipsec: Fix aborted xfrm policy dump crash")
> > > 
> > > My apologies for the noise if this is already under consideration.
> > 
> > It's a bit too big hammer. As Nicolai Stange noticed when we were
> 
> The hammer is just as big as the upstream hammer. Personally I prefer the
> upstream patch; I don't see a reason to deviate from upstream just because
> the upstream solution is more complex than necessary.

Comparing that little patch with the combination of the two commits,
I would say we have a very different idea what "as big as" means. :-)

> > handling this for SLE12 (where fc9e50f5a5a4e would break kABI), it's
> 
> I didn't know that this is even a concern for stable releases. Is there
> some guideline that kABI changes should be avoided in stable releases ?

Not to my knowledge, stable updates break kABI quite often. I just
mentioned it to explain why we had stronger motivation to find another
solution.

Michal Kubecek

^ permalink raw reply

* Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op
From: Shannon Nelson @ 2017-12-03  0:38 UTC (permalink / raw)
  To: Yossi Kuperman, Steffen Klassert
  Cc: Aviv Heller, Herbert Xu, Boris Pismenny, Yevgeny Kliteynik,
	netdev@vger.kernel.org
In-Reply-To: <4767ACB7-EBCF-407C-95C8-7345109D7544@mellanox.com>

On 12/2/2017 2:33 PM, Yossi Kuperman wrote:
> 
> 
>>> On 1 Dec 2017, at 9:09, Steffen Klassert <steffen.klassert@secunet.com> wrote:
>>>
>>> On Tue, Nov 28, 2017 at 07:55:41PM +0200, avivh@mellanox.com wrote:
>>> From: Aviv Heller <avivh@mellanox.com>
>>>
>>> Adding the state to the offload device prior to replay init in
>>> xfrm_state_construct() will result in NULL dereference if a matching
>>> ESP packet is received in between.
>>>
>>> In order to inhibit driver offload logic from processing the state's
>>> packets prior to the xfrm_state object being completely initialized and
>>> added to the SADBs, a new activate() operation was added to inform the
>>> driver the aforementioned conditions have been met.
>>
>> We discussed this already some time ago, and I still think that
>> we should fix this by setting XFRM_STATE_VALID only after the
>> state is fully initialized.
> 
> An upcoming patch will refactor the if statement (encap_type < 0) in xfrm_input, in order to support crypto offload with GRO disabled. Currently it doesn’t work. This entails yet another check for the validity of the state. Resulting in total of 3 copies: 1) for normal traffic, 2) GRO and 3) crypto offload.
> 
> Anyway, IMO it is not right that we (the driver) allow an incoming packet to be delivered while the SA is not yet ready. Rather than checking for an invalid input I prefer to make sure that such a case won’t happen in the first place.
> 
> To complete the picture, there is another patch to the driver which simply drop incoming packets that underwent successful decryption and haven’t been activated yet. Active state merely means that the SA is present in the driver’s hash table.
> 
> We can make a separate patch to set the state to valid once it is fully initialized, it make sense on its own.
> 
> What do you think?
> 

If the SA isn't ready, just don't tell the driver about it.  Please 
don't add yet another state for the driver to track.  This should be as 
simple as possible, and shouldn't be any more complex than the model 
already used by ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid.

sln

^ permalink raw reply

* Re: [PATCH net-next v3 0/8] xdp: make stack perform remove and add selftests
From: Daniel Borkmann @ 2017-12-03  0:21 UTC (permalink / raw)
  To: Jakub Kicinski, netdev; +Cc: oss-drivers, jiri, alexei.starovoitov
In-Reply-To: <20171201230902.20108-1-jakub.kicinski@netronome.com>

On 12/02/2017 12:08 AM, Jakub Kicinski wrote:
> Hi!
> 
> The purpose of this series is to add a software model of BPF offloads
> to make it easier for everyone to test them and make some of the more
> arcane rules and assumptions more clear.
> 
> The series starts with 3 patches aiming to make XDP handling in the
> drivers less error prone.  Currently driver authors have to remember
> to free XDP programs if XDP is active during unregister.  With this
> series the core will disable XDP on its own.  It will take place
> after close, drivers are not expected to perform reconfiguration
> when disabling XDP on a downed device.
> 
> Next two patches add the software netdev driver, followed by a python
> test which exercises all the corner cases which came to my mind.
> 
> Test needs to be run as root.  It will print basic information to
> stdout, but can also create a more detailed log of all commands
> when --log option is passed.  Log is in Emacs Org-mode format.
> 
>   ./tools/testing/selftests/bpf/test_offload.py --log /tmp/log
> 
> Last two patches replace the SR-IOV API implementation of dummy.
> 
> v3:
>  - move the freeing of vfs to release (Phil).
> v2:
>  - free device from the release function;
>  - use bus-based name generatin instead of netdev name.
> v1:
>  - replace the SR-IOV API implementation of dummy;
>  - make the dev_xdp_uninstall() also handle the XDP generic (Daniel).

Series applied to bpf-next, thanks Jakub!

^ permalink raw reply

* Re: Fixing CVE-2017-16939 in v4.4.y and possibly v3.18.y
From: Guenter Roeck @ 2017-12-03  0:20 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: netdev, stable, Greg Kroah-Hartman
In-Reply-To: <20171201194857.2rzwjdv5fcoedvrx@unicorn.suse.cz>

On 12/01/2017 11:48 AM, Michal Kubecek wrote:
> On Thu, Nov 30, 2017 at 10:37:40AM -0800, Guenter Roeck wrote:
>> Hi,
>>
>> The fix for CVE-2017-16939 has been applied to v4.9.y, but not to v4.4.y
>> and older kernels. However, I confirmed that running the published POC
>> (see https://blogs.securiteam.com/index.php/archives/3535) does crash a 4.4
>> kernel.
>>
>> I confirmed that the following two patches fix the problem in v4.4.y.
>> Please consider applying them to v4.4.y (and possibly v3.18.y).
>>
>> fc9e50f5a5a4e ("netlink: add a start callback for starting a netlink dump")
>> 1137b5e2529a8 ("ipsec: Fix aborted xfrm policy dump crash")
>>
>> My apologies for the noise if this is already under consideration.
> 
> It's a bit too big hammer. As Nicolai Stange noticed when we were

The hammer is just as big as the upstream hammer. Personally I prefer the
upstream patch; I don't see a reason to deviate from upstream just because
the upstream solution is more complex than necessary.

> handling this for SLE12 (where fc9e50f5a5a4e would break kABI), it's

I didn't know that this is even a concern for stable releases. Is there
some guideline that kABI changes should be avoided in stable releases ?

Thanks,
Guenter

> much simpler to use the flag we already have in cb->args[0] to let
> xfrm_dump_policy_done() call xfrm_policy_walk_done() only if the walk
> structure has been initialized. Thus all you need is the patch below.
> 
> Michal Kubecek
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 7a5a64e70b4d..c01c7a7eb4d3 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1655,7 +1655,9 @@ static int xfrm_dump_policy_done(struct netlink_callback *cb)
>   	struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *) &cb->args[1];
>   	struct net *net = sock_net(cb->skb->sk);
>   
> -	xfrm_policy_walk_done(walk, net);
> +	/* cb->args[0] is set when walk is initialized */
> +	if (cb->args[0])
> +		xfrm_policy_walk_done(walk, net);
>   	return 0;
>   }
>   
> 

^ permalink raw reply

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
From: David Ahern @ 2017-12-02 22:48 UTC (permalink / raw)
  To: Johannes Berg, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: j, Johannes Berg
In-Reply-To: <20171202202332.10205-1-johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>

On 12/2/17 1:23 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This netlink type is used only for backwards compatibility
> with broken userspace that used the wrong size for a given
> u8 attribute, which is now rejected. It would've been wrong
> before already, since on big endian the wrong value (always
> zero) would be used by the kernel, but we can't break the
> existing deployed userspace - hostapd for example now fails
> to initialize entirely.
> 
> We could try to fix up the big endian problem here, but we
> don't know *how* userspace misbehaved - if using nla_put_u32
> then we could, but we also found a debug tool (which we'll
> ignore for the purposes of this regression) that was putting
> the padding into the length.
> 
> Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
> Signed-off-by: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Hi Johannes:

I have been really busy the past 2 weeks, so have not gotten around to
dealing with this. I was planning to partially revert 28033ae4e0f5 --
change it from failure to log an error message so buggy commands can be
fixed.

David

^ permalink raw reply

* Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op
From: Yossi Kuperman @ 2017-12-02 22:33 UTC (permalink / raw)
  To: Steffen Klassert
  Cc: Aviv Heller, Herbert Xu, Boris Pismenny, Yevgeny Kliteynik,
	netdev@vger.kernel.org
In-Reply-To: <20171201070919.mbyzbgud7ud42uqi@gauss3.secunet.de>



>> On 1 Dec 2017, at 9:09, Steffen Klassert <steffen.klassert@secunet.com> wrote:
>> 
>> On Tue, Nov 28, 2017 at 07:55:41PM +0200, avivh@mellanox.com wrote:
>> From: Aviv Heller <avivh@mellanox.com>
>> 
>> Adding the state to the offload device prior to replay init in
>> xfrm_state_construct() will result in NULL dereference if a matching
>> ESP packet is received in between.
>> 
>> In order to inhibit driver offload logic from processing the state's
>> packets prior to the xfrm_state object being completely initialized and
>> added to the SADBs, a new activate() operation was added to inform the
>> driver the aforementioned conditions have been met.
> 
> We discussed this already some time ago, and I still think that
> we should fix this by setting XFRM_STATE_VALID only after the
> state is fully initialized.

An upcoming patch will refactor the if statement (encap_type < 0) in xfrm_input, in order to support crypto offload with GRO disabled. Currently it doesn’t work. This entails yet another check for the validity of the state. Resulting in total of 3 copies: 1) for normal traffic, 2) GRO and 3) crypto offload.

Anyway, IMO it is not right that we (the driver) allow an incoming packet to be delivered while the SA is not yet ready. Rather than checking for an invalid input I prefer to make sure that such a case won’t happen in the first place.

To complete the picture, there is another patch to the driver which simply drop incoming packets that underwent successful decryption and haven’t been activated yet. Active state merely means that the SA is present in the driver’s hash table.

We can make a separate patch to set the state to valid once it is fully initialized, it make sense on its own.

What do you think?




^ permalink raw reply

* Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op
From: Shannon Nelson @ 2017-12-02 22:11 UTC (permalink / raw)
  To: avivh, Steffen Klassert
  Cc: Herbert Xu, Boris Pismenny, Yossi Kuperman, Yevgeny Kliteynik,
	netdev
In-Reply-To: <e71364ac-1abb-71a6-c0a5-6f859528a0c7@oracle.com>

On 12/1/2017 11:47 AM, Shannon Nelson wrote:
> On 11/28/2017 9:55 AM, avivh@mellanox.com wrote:
>> From: Aviv Heller <avivh@mellanox.com>
>>
>> Adding the state to the offload device prior to replay init in
>> xfrm_state_construct() will result in NULL dereference if a matching
>> ESP packet is received in between.
>>
>> In order to inhibit driver offload logic from processing the state's
>> packets prior to the xfrm_state object being completely initialized and
>> added to the SADBs, a new activate() operation was added to inform the
>> driver the aforementioned conditions have been met.
> 
> Are there also conditions where you would want to temporarily 
> deactivate, or pause, the incoming driver offload, followed then by 
> another activate?
> 
> sln

Instead of setting up a half-ready state that needs the activate() 
operation to finish, can we instead just move the xfrm_dev_state_add() 
call to after the xfrm_init_replay()?  Especially since this really only 
makes sense for the inbound, and makes no sense for the outbound path.

sln

> 
>>
>> Signed-off-by: Aviv Heller <avivh@mellanox.com>
>> Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
>> ---
>> v1 -> v2:
>>     - Separate to state addition and then activation, instead
>>       of relocating dev state addition call.
>> ---
>>   include/linux/netdevice.h |  1 +
>>   include/net/xfrm.h        | 12 ++++++++++++
>>   net/xfrm/xfrm_user.c      |  5 +++++
>>   3 files changed, 18 insertions(+)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 2eaac7d..c6ca356 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -819,6 +819,7 @@ struct netdev_xdp {
>>   #ifdef CONFIG_XFRM_OFFLOAD
>>   struct xfrmdev_ops {
>>       int    (*xdo_dev_state_add) (struct xfrm_state *x);
>> +    void    (*xdo_dev_state_activate) (struct xfrm_state *x);
>>       void    (*xdo_dev_state_delete) (struct xfrm_state *x);
>>       void    (*xdo_dev_state_free) (struct xfrm_state *x);
>>       bool    (*xdo_dev_offload_ok) (struct sk_buff *skb,
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index e015e16..324374e 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -1877,6 +1877,14 @@ static inline bool xfrm_dst_offload_ok(struct 
>> dst_entry *dst)
>>       return false;
>>   }
>> +static inline void xfrm_dev_state_activate(struct xfrm_state *x)
>> +{
>> +    struct xfrm_state_offload *xso = &x->xso;
>> +
>> +    if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_activate)
>> +        xso->dev->xfrmdev_ops->xdo_dev_state_activate(x);
>> +}
>> +
>>   static inline void xfrm_dev_state_delete(struct xfrm_state *x)
>>   {
>>       struct xfrm_state_offload *xso = &x->xso;
>> @@ -1907,6 +1915,10 @@ static inline int xfrm_dev_state_add(struct net 
>> *net, struct xfrm_state *x, stru
>>       return 0;
>>   }
>> +static inline void xfrm_dev_state_activate(struct xfrm_state *x)
>> +{
>> +}
>> +
>>   static inline void xfrm_dev_state_delete(struct xfrm_state *x)
>>   {
>>   }
>> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
>> index e44a0fe..d06f579 100644
>> --- a/net/xfrm/xfrm_user.c
>> +++ b/net/xfrm/xfrm_user.c
>> @@ -662,6 +662,11 @@ static int xfrm_add_sa(struct sk_buff *skb, 
>> struct nlmsghdr *nlh,
>>           goto out;
>>       }
>> +    spin_lock_bh(&x->lock);
>> +    if (x->km.state == XFRM_STATE_VALID)
>> +        xfrm_dev_state_activate(x);
>> +    spin_unlock_bh(&x->lock);
>> +
>>       c.seq = nlh->nlmsg_seq;
>>       c.portid = nlh->nlmsg_pid;
>>       c.event = nlh->nlmsg_type;
>>

^ permalink raw reply

* Re: netfilter: xt_bpf: Fix XT_BPF_MODE_FD_PINNED mode of 'xt_bpf_info_v1'
From: Al Viro @ 2017-12-02 22:08 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Kees Cook, Shmulik Ladkani, Willem de Bruijn, Pablo Neira Ayuso,
	Linus Torvalds, David Miller, LKML, Network Development,
	Christoph Hellwig, Thomas Garnier, Jann Horn
In-Reply-To: <20171202184850.GQ21978@ZenIV.linux.org.uk>

On Sat, Dec 02, 2017 at 06:48:50PM +0000, Al Viro wrote:
> On Fri, Dec 01, 2017 at 09:47:00PM +0100, Daniel Borkmann wrote:
> 
> > > Might want to replace security_path_mknod() with something saner, while we are
> > > at it.
> > > 
> > > Objections?
> > 
> > No, thanks for looking into this, and sorry for this fugly hack! :( Not
> > that this doesn't make it any better, but I think back then I took it
> > over from mqueue implementation ... should have known better and looking
> > into making this generic instead, sigh. The above looks good to me, so
> > no objections from my side and thanks for working on it!
> > 
> > > PS: mqueue.c would also benefit from such primitive - do_create() there would
> > > simply pass attr as callback's argument into vfs_mkobj(), with callback being
> > > the guts of mqueue_create()...
> 
> OK...  See vfs.git#untested.mkobj; it really needs testing, though - mq_open(2)
> passes LTP tests, but that's not saying much, and BPF side is completely
> untested.

... and FWIW, completely untested patch for net/netfilter/xt_bpf.c follows:

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e55e4255a210..a7000e4775e7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -514,6 +514,9 @@ static inline struct bpf_prog *bpf_prog_get_type(u32 ufd,
 	return bpf_prog_get_type_dev(ufd, type, false);
 }
 
+struct bpf_prog *bpf_prog_get_type_path(const char *name, enum bpf_prog_type type);
+bool bpf_prog_get_ok(struct bpf_prog *, enum bpf_prog_type *, bool);
+
 int bpf_prog_offload_compile(struct bpf_prog *prog);
 void bpf_prog_offload_destroy(struct bpf_prog *prog);
 
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index 2b75faccc771..9d1050dc2a7a 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -364,6 +364,45 @@ int bpf_obj_get_user(const char __user *pathname, int flags)
 }
 EXPORT_SYMBOL_GPL(bpf_obj_get_user);
 
+static struct bpf_prog *__get_prog_inode(struct inode *inode, enum bpf_prog_type type)
+{
+	struct bpf_prog *prog;
+	int ret = inode_permission(inode, MAY_READ | MAY_WRITE);
+	if (ret)
+		return ERR_PTR(ret);
+
+	if (inode->i_op == &bpf_map_iops)
+		return ERR_PTR(-EINVAL);
+	if (inode->i_op != &bpf_prog_iops)
+		return ERR_PTR(-EACCES);
+
+	prog = inode->i_private;
+
+	ret = security_bpf_prog(prog);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	if (!bpf_prog_get_ok(prog, &type, false))
+		return ERR_PTR(-EINVAL);
+
+	return bpf_prog_inc(prog);
+}
+
+struct bpf_prog *bpf_prog_get_type_path(const char *name, enum bpf_prog_type type)
+{
+	struct bpf_prog *prog;
+	struct path path;
+	int ret = kern_path(name, LOOKUP_FOLLOW, &path);
+	if (ret)
+		return ERR_PTR(ret);
+	prog = __get_prog_inode(d_backing_inode(path.dentry), type);
+	if (!IS_ERR(prog))
+		touch_atime(&path);
+	path_put(&path);
+	return prog;
+}
+EXPORT_SYMBOL(bpf_prog_get_type_path);
+
 static void bpf_evict_inode(struct inode *inode)
 {
 	enum bpf_type type;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 2c4cfeaa8d5e..5cb783fc8224 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1057,7 +1057,7 @@ struct bpf_prog *bpf_prog_inc_not_zero(struct bpf_prog *prog)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_inc_not_zero);
 
-static bool bpf_prog_get_ok(struct bpf_prog *prog,
+bool bpf_prog_get_ok(struct bpf_prog *prog,
 			    enum bpf_prog_type *attach_type, bool attach_drv)
 {
 	/* not an attachment, just a refcount inc, always allow */
diff --git a/net/netfilter/xt_bpf.c b/net/netfilter/xt_bpf.c
index 041da0d9c06f..fa2ca0a13619 100644
--- a/net/netfilter/xt_bpf.c
+++ b/net/netfilter/xt_bpf.c
@@ -52,18 +52,8 @@ static int __bpf_mt_check_fd(int fd, struct bpf_prog **ret)
 
 static int __bpf_mt_check_path(const char *path, struct bpf_prog **ret)
 {
-	mm_segment_t oldfs = get_fs();
-	int retval, fd;
-
-	set_fs(KERNEL_DS);
-	fd = bpf_obj_get_user(path, 0);
-	set_fs(oldfs);
-	if (fd < 0)
-		return fd;
-
-	retval = __bpf_mt_check_fd(fd, ret);
-	sys_close(fd);
-	return retval;
+	*ret = bpf_prog_get_type_path(path, BPF_PROG_TYPE_SOCKET_FILTER);
+	return PTR_ERR_OR_ZERO(*ret);
 }
 
 static int bpf_mt_check(const struct xt_mtchk_param *par)

^ permalink raw reply related

* [RfC net-next 3/3] net: phy: realtek: add more interrupt bits for RTL8211E and RTL8211F
From: Martin Blumenstingl @ 2017-12-02 22:06 UTC (permalink / raw)
  To: netdev
  Cc: f.fainelli, andrew, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh, Martin Blumenstingl
In-Reply-To: <20171202220650.23391-1-martin.blumenstingl@googlemail.com>

This documents a few more bits in the RTL821x_INER register for RTL8211E
and RTL8211F. These are added only to document them (as no public
datasheets are available for these PHYs), they are currently not used.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 961165d128d6..a793c35cbaae 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -24,7 +24,14 @@
 #define RTL821x_INER				0x12
 #define RTL8211B_INER_INIT			0x6400
 #define RTL8211E_INER_LINK_STATUS		BIT(10)
+#define RTL8211E_INER_ANEG_COMPLETED		BIT(11)
+#define RTL8211E_INER_PAGE_RECEIVED		BIT(12)
+#define RTL8211E_INER_ANEG_ERROR		BIT(15)
 #define RTL8211F_INER_LINK_STATUS		BIT(4)
+#define RTL8211F_INER_PHY_REGISTER_ACCESSIBLE	BIT(5)
+#define RTL8211F_INER_WOL_PME			BIT(7)
+#define RTL8211F_INER_ALDPS_STATE_CHANGE	BIT(9)
+#define RTL8211F_INER_JABBER			BIT(10)
 
 #define RTL821x_INSR				0x13
 
-- 
2.15.1

^ permalink raw reply related

* [RfC net-next 2/3] net: phy: realtek: configure the INTB pin on RTL8211F
From: Martin Blumenstingl @ 2017-12-02 22:06 UTC (permalink / raw)
  To: netdev
  Cc: f.fainelli, andrew, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh, Martin Blumenstingl
In-Reply-To: <20171202220650.23391-1-martin.blumenstingl@googlemail.com>

The interrupt pin on the RTL8211F PHY can be used in two different
modes:
INTB
- the default mode of the PHY
- interrupts can be configured through page 0xa42 register RTL821x_INER
- interrupts can be ACK'ed through RTL8211F_INSR
- it acts as a level-interrupt which is active low
- Wake-on-LAN "wakeup" status is available in RTL8211F_INSR bit 7

PMEB:
- special mode for Wake-on-LAN
- interrupts configured through page 0xa42 register RTL821x_INER are
  disabled
- it supports a "pulse low" waveform for the interrupt

For now we simply force the pin into INTB mode since the PHY driver does
not support Wake-on-LAN yet.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index d4e7f249a4bc..961165d128d6 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -40,6 +40,9 @@
 #define RTL8201F_ISR				0x1e
 #define RTL8201F_IER				0x13
 
+#define RTL8211F_INTBCR				0x16
+#define RTL8211F_INTBCR_INTB_PMEB		BIT(5)
+
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
@@ -161,12 +164,32 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 
 static int rtl8211f_config_intr(struct phy_device *phydev)
 {
+	int err;
 	u16 val;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		/*
+		 * The interrupt pin has two functions:
+		 * 0: INTB: it acts as interrupt pin which can be configured
+		 *    through RTL821x_INER and the status can be read through
+		 *    RTL8211F_INSR
+		 * 1: PMEB: a special "Power Management Event" mode for
+		 *    Wake-on-LAN operation (with support for a "pulse low"
+		 *    wave format). Interrupts configured through RTL821x_INER
+		 *    will not work in this mode
+		 *
+		 * select INTB mode in the "INTB pin control" register to
+		 * ensure that the interrupt pin is in the correct mode.
+		 */
+		err = rtl8211x_page_mask_bits(phydev, 0xd40, RTL8211F_INTBCR,
+					      RTL8211F_INTBCR_INTB_PMEB, 0);
+		if (err)
+			return err;
+
 		val = RTL8211F_INER_LINK_STATUS;
-	else
+	} else {
 		val = 0;
+	}
 
 	return rtl8211x_page_write(phydev, 0xa42, RTL821x_INER, val);
 }
-- 
2.15.1

^ permalink raw reply related

* [RfC net-next 1/3] net: phy: realtek: add support for configuring the RX delay on RTL8211F
From: Martin Blumenstingl @ 2017-12-02 22:06 UTC (permalink / raw)
  To: netdev
  Cc: f.fainelli, andrew, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh, Martin Blumenstingl
In-Reply-To: <20171202220650.23391-1-martin.blumenstingl@googlemail.com>

On RTL8211F the RX delay can also be enabled/disabled.
The overall behavior of the RX delay is similar to the behavior of the
TX delay, which was already supported by the driver.

The RX delay (similar to the TX delay) may be enabled using hardware pin
strapping. If the MAC already configures the RX delay (if required) then
the RX delay generated by the RTL8211F PHY has to be turned off.

While here, update the comment regarding the TX delay why it has to be
enabled or disabled within the driver.
Also avoid code-duplication by extracting the code to mask/unmask bits
in a paged register into a new rtl8211x_page_mask_bits helper function.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 55 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 5416ec5af042..d4e7f249a4bc 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -32,7 +32,10 @@
 
 #define RTL8211F_INSR				0x1d
 
-#define RTL8211F_TX_DELAY			BIT(8)
+#define RTL8211F_RX_DELAY_REG			0x15
+#define RTL8211F_RX_DELAY_EN			BIT(3)
+#define RTL8211F_TX_DELAY_REG			0x11
+#define RTL8211F_TX_DELAY_EN			BIT(8)
 
 #define RTL8201F_ISR				0x1e
 #define RTL8201F_IER				0x13
@@ -74,6 +77,23 @@ static int rtl8211x_page_write(struct phy_device *phydev, u16 page,
 	return ret;
 }
 
+static int rtl8211x_page_mask_bits(struct phy_device *phydev, u16 page,
+				   u16 address, u16 mask, u16 set)
+{
+	int ret;
+	u16 val;
+
+	ret = rtl8211x_page_read(phydev, page, address);
+	if (ret < 0)
+		return ret;
+
+	val = ret & 0xffff;
+	val &= ~mask;
+	val |= (set & mask);
+
+	return rtl8211x_page_write(phydev, page, address, val);
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -160,20 +180,35 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	ret = rtl8211x_page_read(phydev, 0xd08, 0x11);
-	if (ret < 0)
-		return ret;
+	/*
+	 * enable TX-delay for rgmii-id and rgmii-txid, otherwise disable it.
+	 * this is needed because it can be enabled by pin strapping and
+	 * conflict with the TX-delay configured by the MAC.
+	 */
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+		val = RTL8211F_TX_DELAY_EN;
+	else
+		val = 0;
 
-	val = ret & 0xffff;
+	ret = rtl8211x_page_mask_bits(phydev, 0xd08, RTL8211F_TX_DELAY_REG,
+				      RTL8211F_TX_DELAY_EN, val);
+	if (ret)
+		return ret;
 
-	/* enable TX-delay for rgmii-id and rgmii-txid, otherwise disable it */
+	/*
+	 * enable RX-delay for rgmii-id and rgmii-rxid, otherwise disable it.
+	 * this is needed because it can be enabled by pin strapping and
+	 * conflict with the RX-delay configured by the MAC.
+	 */
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
-		val |= RTL8211F_TX_DELAY;
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+		val = RTL8211F_RX_DELAY_EN;
 	else
-		val &= ~RTL8211F_TX_DELAY;
+		val = 0;
 
-	ret = rtl8211x_page_write(phydev, 0xd08, 0x11, val);
+	ret = rtl8211x_page_mask_bits(phydev, 0xd08, RTL8211F_RX_DELAY_REG,
+				      RTL8211F_RX_DELAY_EN, val);
 	if (ret)
 		return ret;
 
-- 
2.15.1

^ permalink raw reply related

* [RfC net-next 0/3] RTL8211F Ethernet PHY "documentation"
From: Martin Blumenstingl @ 2017-12-02 22:06 UTC (permalink / raw)
  To: netdev
  Cc: f.fainelli, andrew, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh, Martin Blumenstingl

A recent patch from Heiner made me curious if the RTL8211F part of
the realtek.c PHY driver is correct: [0]
I contacted Realtek and asked if we could get a datasheet for the
RTL8211F PHY. it seems that the full datasheet can only be obtained
with an NDA. however, the contact at Realtek kindly answered all
questions I had regarding the RTL8211F PHY (thank you very much
again!). I am not permitted to share the exact answers I received,
but I am allowed to put them into my own words.

This series is a result of the conversation with Realtek: the main
intention behind this series is to *document* the information I got.
I am not aware of any board which needs the RX delay, has a problem
with the INTB pin configuration or requires any of the additional
interrupts.

I only tested that these patches don't break existing functionality
on Khadas VIM2 board with RTL8211F PHY.

PS: I also received information about the RTL8211E PHY's RX and TX
delay configuration. however, I don't understand that part of the
datasheet so I did not add any #defines in patch #3. the datasheet
says that:
- TX delay is configured through bit 12 and bit 13 in register 0x1c
  in page 0xa4 (value 1 = enabled, 0 = disabled)
- RX delay is configured through bit 11 and bit 13 in register 0x1c
  in page 0xa4 (value 1 = enabled, 0 = disabled)
I don't have any board with a RTL8211E PHY, so I could not test this
part at all. thus I don't know why bit 13 is listed for both, RX and
TX delay.

I do not expect that this series is applied. if someone is interested
in testing this: it applies on top of my other series:
"Realtek Ethernet PHY driver improvements" [1]


[0] https://www.spinics.net/lists/netdev/msg466661.html
[1] https://marc.info/?l=linux-netdev&m=151225151410593&w=2

Martin Blumenstingl (3):
  net: phy: realtek: add support for configuring the RX delay on
    RTL8211F
  net: phy: realtek: configure the INTB pin on RTL8211F
  net: phy: realtek: add more interrupt bits for RTL8211E and RTL8211F

 drivers/net/phy/realtek.c | 89 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 77 insertions(+), 12 deletions(-)

-- 
2.15.1

^ permalink raw reply

* [PATCH net-next 5/5] net: phy: realtek: add utility functions to read/write page addresses
From: Martin Blumenstingl @ 2017-12-02 21:51 UTC (permalink / raw)
  To: netdev
  Cc: f.fainelli, andrew, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh, Martin Blumenstingl
In-Reply-To: <20171202215128.20202-1-martin.blumenstingl@googlemail.com>

Realtek PHYs implement the concept of so-called "extension pages". The
reason for this is probably because these PHYs expose more registers
than available in the standard address range.
After all read/write operations on such a page are done the driver
should switch back to page 0 where the standard MII registers (such as
MII_BMCR) are available.

When referring to such a register the datasheets of RTL8211E and
RTL8211F always specify:
- the page / "ext. page" which has to be written to RTL821x_PAGE_SELECT
- an address (sometimes also called reg)

These new utility functions make the existing code easier to read since
it removes some duplication (switching back to page 0 is done within the
new helpers for example).

No functional changes are intended.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 83 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 30 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index d6868e8daaab..5416ec5af042 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -41,6 +41,39 @@ MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
 
+static int rtl8211x_page_read(struct phy_device *phydev, u16 page, u16 address)
+{
+	int ret;
+
+	ret = phy_write(phydev, RTL821x_PAGE_SELECT, page);
+	if (ret)
+		return ret;
+
+	ret = phy_read(phydev, address);
+
+	/* restore to default page 0 */
+	phy_write(phydev, RTL821x_PAGE_SELECT, 0x0);
+
+	return ret;
+}
+
+static int rtl8211x_page_write(struct phy_device *phydev, u16 page,
+			       u16 address, u16 val)
+{
+	int ret;
+
+	ret = phy_write(phydev, RTL821x_PAGE_SELECT, page);
+	if (ret)
+		return ret;
+
+	ret = phy_write(phydev, address, val);
+
+	/* restore to default page 0 */
+	phy_write(phydev, RTL821x_PAGE_SELECT, 0x0);
+
+	return ret;
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -63,31 +96,21 @@ static int rtl8211f_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
 
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa43);
-	err = phy_read(phydev, RTL8211F_INSR);
-	/* restore to default page 0 */
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0x0);
+	err = rtl8211x_page_read(phydev, 0xa43, RTL8211F_INSR);
 
 	return (err < 0) ? err : 0;
 }
 
 static int rtl8201_config_intr(struct phy_device *phydev)
 {
-	int err;
-
-	/* switch to page 7 */
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0x7);
+	u16 val;
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		err = phy_write(phydev, RTL8201F_IER,
-				BIT(13) | BIT(12) | BIT(11));
+		val = BIT(13) | BIT(12) | BIT(11);
 	else
-		err = phy_write(phydev, RTL8201F_IER, 0);
+		val = 0;
 
-	/* restore to default page 0 */
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0x0);
-
-	return err;
+	return rtl8211x_page_write(phydev, 0x7, RTL8201F_IER, val);
 }
 
 static int rtl8211b_config_intr(struct phy_device *phydev)
@@ -118,41 +141,41 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 
 static int rtl8211f_config_intr(struct phy_device *phydev)
 {
-	int err;
+	u16 val;
 
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa42);
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		err = phy_write(phydev, RTL821x_INER,
-				RTL8211F_INER_LINK_STATUS);
+		val = RTL8211F_INER_LINK_STATUS;
 	else
-		err = phy_write(phydev, RTL821x_INER, 0);
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0);
+		val = 0;
 
-	return err;
+	return rtl8211x_page_write(phydev, 0xa42, RTL821x_INER, val);
 }
 
 static int rtl8211f_config_init(struct phy_device *phydev)
 {
 	int ret;
-	u16 reg;
+	u16 val;
 
 	ret = genphy_config_init(phydev);
 	if (ret < 0)
 		return ret;
 
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0xd08);
-	reg = phy_read(phydev, 0x11);
+	ret = rtl8211x_page_read(phydev, 0xd08, 0x11);
+	if (ret < 0)
+		return ret;
+
+	val = ret & 0xffff;
 
 	/* enable TX-delay for rgmii-id and rgmii-txid, otherwise disable it */
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
 	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
-		reg |= RTL8211F_TX_DELAY;
+		val |= RTL8211F_TX_DELAY;
 	else
-		reg &= ~RTL8211F_TX_DELAY;
+		val &= ~RTL8211F_TX_DELAY;
 
-	phy_write(phydev, 0x11, reg);
-	/* restore to default page 0 */
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0x0);
+	ret = rtl8211x_page_write(phydev, 0xd08, 0x11, val);
+	if (ret)
+		return ret;
 
 	return 0;
 }
-- 
2.15.1

^ permalink raw reply related

* [PATCH net-next 4/5] net: phy: realtek: use the same indentation for all #defines
From: Martin Blumenstingl @ 2017-12-02 21:51 UTC (permalink / raw)
  To: netdev
  Cc: f.fainelli, andrew, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh, Martin Blumenstingl
In-Reply-To: <20171202215128.20202-1-martin.blumenstingl@googlemail.com>

This simply makes the code easier to read. No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index da263a92d6b1..d6868e8daaab 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -17,24 +17,25 @@
 #include <linux/phy.h>
 #include <linux/module.h>
 
-#define RTL821x_PHYSR		0x11
-#define RTL821x_PHYSR_DUPLEX	BIT(13)
-#define RTL821x_PHYSR_SPEED	GENMASK(15, 14)
+#define RTL821x_PHYSR				0x11
+#define RTL821x_PHYSR_DUPLEX			BIT(13)
+#define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
 
-#define RTL821x_INER		0x12
-#define RTL8211B_INER_INIT	0x6400
-#define RTL8211E_INER_LINK_STATUS	BIT(10)
-#define RTL8211F_INER_LINK_STATUS	BIT(4)
+#define RTL821x_INER				0x12
+#define RTL8211B_INER_INIT			0x6400
+#define RTL8211E_INER_LINK_STATUS		BIT(10)
+#define RTL8211F_INER_LINK_STATUS		BIT(4)
 
-#define RTL821x_INSR		0x13
+#define RTL821x_INSR				0x13
 
-#define RTL821x_PAGE_SELECT	0x1f
+#define RTL821x_PAGE_SELECT			0x1f
 
-#define RTL8211F_INSR		0x1d
-#define RTL8211F_TX_DELAY	BIT(8)
+#define RTL8211F_INSR				0x1d
 
-#define RTL8201F_ISR		0x1e
-#define RTL8201F_IER		0x13
+#define RTL8211F_TX_DELAY			BIT(8)
+
+#define RTL8201F_ISR				0x1e
+#define RTL8201F_IER				0x13
 
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
-- 
2.15.1

^ permalink raw reply related

* [PATCH net-next 3/5] net: phy: realtek: group all register bit #defines for RTL821x_INER
From: Martin Blumenstingl @ 2017-12-02 21:51 UTC (permalink / raw)
  To: netdev
  Cc: f.fainelli, andrew, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh, Martin Blumenstingl
In-Reply-To: <20171202215128.20202-1-martin.blumenstingl@googlemail.com>

This simply moves all register bit #defines which describe the (PHY
specific) bits in the RTL821x_INER right below the RTL821x_INER register
definition. This makes it easier to spot which registers and bits belong
together.
No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 59f0688e4d28..da263a92d6b1 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -20,13 +20,16 @@
 #define RTL821x_PHYSR		0x11
 #define RTL821x_PHYSR_DUPLEX	BIT(13)
 #define RTL821x_PHYSR_SPEED	GENMASK(15, 14)
+
 #define RTL821x_INER		0x12
 #define RTL8211B_INER_INIT	0x6400
+#define RTL8211E_INER_LINK_STATUS	BIT(10)
+#define RTL8211F_INER_LINK_STATUS	BIT(4)
+
 #define RTL821x_INSR		0x13
+
 #define RTL821x_PAGE_SELECT	0x1f
-#define RTL8211E_INER_LINK_STATUS	BIT(10)
 
-#define RTL8211F_INER_LINK_STATUS	BIT(4)
 #define RTL8211F_INSR		0x1d
 #define RTL8211F_TX_DELAY	BIT(8)
 
-- 
2.15.1

^ permalink raw reply related

* [PATCH net-next 2/5] net: phy: realtek: rename RTL821x_INER_INIT to RTL8211B_INER_INIT
From: Martin Blumenstingl @ 2017-12-02 21:51 UTC (permalink / raw)
  To: netdev
  Cc: f.fainelli, andrew, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh, Martin Blumenstingl
In-Reply-To: <20171202215128.20202-1-martin.blumenstingl@googlemail.com>

This macro is only used by the RTL8211B code. RTL8211E and RTL8211F both
use other bits to initialize the RTL821x_INER register.
No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 9708aa9c58dd..59f0688e4d28 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -21,7 +21,7 @@
 #define RTL821x_PHYSR_DUPLEX	BIT(13)
 #define RTL821x_PHYSR_SPEED	GENMASK(15, 14)
 #define RTL821x_INER		0x12
-#define RTL821x_INER_INIT	0x6400
+#define RTL8211B_INER_INIT	0x6400
 #define RTL821x_INSR		0x13
 #define RTL821x_PAGE_SELECT	0x1f
 #define RTL8211E_INER_LINK_STATUS	BIT(10)
@@ -92,7 +92,7 @@ static int rtl8211b_config_intr(struct phy_device *phydev)
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
 		err = phy_write(phydev, RTL821x_INER,
-				RTL821x_INER_INIT);
+				RTL8211B_INER_INIT);
 	else
 		err = phy_write(phydev, RTL821x_INER, 0);
 
-- 
2.15.1

^ permalink raw reply related

* [PATCH net-next 1/5] net: phy: realtek: use the BIT and GENMASK macros
From: Martin Blumenstingl @ 2017-12-02 21:51 UTC (permalink / raw)
  To: netdev
  Cc: f.fainelli, andrew, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh, Martin Blumenstingl
In-Reply-To: <20171202215128.20202-1-martin.blumenstingl@googlemail.com>

This makes it easier to compare the #defines with the datasheets.
No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index eda0a6e86918..9708aa9c58dd 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -13,21 +13,22 @@
  * option) any later version.
  *
  */
+#include <linux/bitops.h>
 #include <linux/phy.h>
 #include <linux/module.h>
 
 #define RTL821x_PHYSR		0x11
-#define RTL821x_PHYSR_DUPLEX	0x2000
-#define RTL821x_PHYSR_SPEED	0xc000
+#define RTL821x_PHYSR_DUPLEX	BIT(13)
+#define RTL821x_PHYSR_SPEED	GENMASK(15, 14)
 #define RTL821x_INER		0x12
 #define RTL821x_INER_INIT	0x6400
 #define RTL821x_INSR		0x13
 #define RTL821x_PAGE_SELECT	0x1f
-#define RTL8211E_INER_LINK_STATUS 0x400
+#define RTL8211E_INER_LINK_STATUS	BIT(10)
 
-#define RTL8211F_INER_LINK_STATUS 0x0010
+#define RTL8211F_INER_LINK_STATUS	BIT(4)
 #define RTL8211F_INSR		0x1d
-#define RTL8211F_TX_DELAY	0x100
+#define RTL8211F_TX_DELAY	BIT(8)
 
 #define RTL8201F_ISR		0x1e
 #define RTL8201F_IER		0x13
-- 
2.15.1

^ permalink raw reply related

* [PATCH net-next 0/5] Realtek Ethernet PHY driver improvements
From: Martin Blumenstingl @ 2017-12-02 21:51 UTC (permalink / raw)
  To: netdev
  Cc: f.fainelli, andrew, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh, Martin Blumenstingl

This series provides some small improvements and cleanups for the
Realtek Ethernet PHY driver.
None of the patches in this series should change any functionality.
The goal is to make the code a bit easier to read by:
- re-using the BIT and GENMASK macros (which makes it easier to compare
  the #defines in the kernel with the values from the datasheets)
- rename a #define from a generic name to a PHY-specific name since it's
  only used for one specific PHY
- logically group the register #defines and their register bit #defines
  together
- indentation cleanups
- removed some code duplicating for reading/writing registers on a
  Realtek specific "page"


Martin Blumenstingl (5):
  net: phy: realtek: use the BIT and GENMASK macros
  net: phy: realtek: rename RTL821x_INER_INIT to RTL8211B_INER_INIT
  net: phy: realtek: group all register bit #defines for RTL821x_INER
  net: phy: realtek: use the same indentation for all #defines
  net: phy: realtek: add utility functions to read/write page addresses

 drivers/net/phy/realtek.c | 116 ++++++++++++++++++++++++++++------------------
 1 file changed, 72 insertions(+), 44 deletions(-)

-- 
2.15.1

^ permalink raw reply

* [PATCH net-next 4/4] rtnetlink: remove __rtnl_register
From: Florian Westphal @ 2017-12-02 20:44 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal
In-Reply-To: <20171202204408.4166-1-fw@strlen.de>

This removes __rtnl_register and switches callers to either
rtnl_register or rtnl_register_module.

Also, rtnl_register() will now print an error if memory allocation
failed rather than panic the kernel.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/rtnetlink.h |  2 --
 net/core/rtnetlink.c    | 33 ++++++++-------------------------
 net/ipv6/addrconf.c     | 44 ++++++++++++++++++++++++++++++--------------
 net/ipv6/addrlabel.c    | 13 ++++++-------
 net/ipv6/ip6_fib.c      |  4 ++--
 net/ipv6/route.c        | 20 +++++++++++++++-----
 6 files changed, 61 insertions(+), 55 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index e326b3f9eb5f..14b6b3af8918 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -13,8 +13,6 @@ enum rtnl_link_flags {
 	RTNL_FLAG_DOIT_UNLOCKED = 1,
 };
 
-int __rtnl_register(int protocol, int msgtype,
-		    rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 int rtnl_register_module(struct module *owner, int protocol, int msgtype,
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index de6390365c90..fb2d61df1e2f 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -238,7 +238,7 @@ int rtnl_register_module(struct module *owner,
 EXPORT_SYMBOL_GPL(rtnl_register_module);
 
 /**
- * __rtnl_register - Register a rtnetlink message type
+ * rtnl_register - Register a rtnetlink message type
  * @protocol: Protocol family or PF_UNSPEC
  * @msgtype: rtnetlink message type
  * @doit: Function pointer called for each request message
@@ -252,35 +252,18 @@ EXPORT_SYMBOL_GPL(rtnl_register_module);
  * The special protocol family PF_UNSPEC may be used to define fallback
  * function pointers for the case when no entry for the specific protocol
  * family exists.
- *
- * Returns 0 on success or a negative error code.
- */
-int __rtnl_register(int protocol, int msgtype,
-		    rtnl_doit_func doit, rtnl_dumpit_func dumpit,
-		    unsigned int flags)
-{
-	return rtnl_register_internal(NULL, protocol, msgtype,
-				      doit, dumpit, flags);
-}
-EXPORT_SYMBOL_GPL(__rtnl_register);
-
-/**
- * rtnl_register - Register a rtnetlink message type
- *
- * Identical to __rtnl_register() but panics on failure. This is useful
- * as failure of this function is very unlikely, it can only happen due
- * to lack of memory when allocating the chain to store all message
- * handlers for a protocol. Meant for use in init functions where lack
- * of memory implies no sense in continuing.
  */
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func doit, rtnl_dumpit_func dumpit,
 		   unsigned int flags)
 {
-	if (__rtnl_register(protocol, msgtype, doit, dumpit, flags) < 0)
-		panic("Unable to register rtnetlink message handler, "
-		      "protocol = %d, message type = %d\n",
-		      protocol, msgtype);
+	int err;
+
+	err = rtnl_register_internal(NULL, protocol, msgtype, doit, dumpit,
+				     flags);
+	if (err)
+		pr_err("Unable to register rtnetlink message handler, "
+		       "protocol = %d, message type = %d\n", protocol, msgtype);
 }
 EXPORT_SYMBOL_GPL(rtnl_register);
 
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f49bd7897e95..a5ad8425551a 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -6595,27 +6595,43 @@ int __init addrconf_init(void)
 
 	rtnl_af_register(&inet6_ops);
 
-	err = __rtnl_register(PF_INET6, RTM_GETLINK, NULL, inet6_dump_ifinfo,
-			      0);
+	err = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETLINK,
+				   NULL, inet6_dump_ifinfo, 0);
 	if (err < 0)
 		goto errout;
 
-	/* Only the first call to __rtnl_register can fail */
-	__rtnl_register(PF_INET6, RTM_NEWADDR, inet6_rtm_newaddr, NULL, 0);
-	__rtnl_register(PF_INET6, RTM_DELADDR, inet6_rtm_deladdr, NULL, 0);
-	__rtnl_register(PF_INET6, RTM_GETADDR, inet6_rtm_getaddr,
-			inet6_dump_ifaddr, RTNL_FLAG_DOIT_UNLOCKED);
-	__rtnl_register(PF_INET6, RTM_GETMULTICAST, NULL,
-			inet6_dump_ifmcaddr, 0);
-	__rtnl_register(PF_INET6, RTM_GETANYCAST, NULL,
-			inet6_dump_ifacaddr, 0);
-	__rtnl_register(PF_INET6, RTM_GETNETCONF, inet6_netconf_get_devconf,
-			inet6_netconf_dump_devconf, RTNL_FLAG_DOIT_UNLOCKED);
-
+	err = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_NEWADDR,
+				   inet6_rtm_newaddr, NULL, 0);
+	if (err < 0)
+		goto errout;
+	err = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_DELADDR,
+				   inet6_rtm_deladdr, NULL, 0);
+	if (err < 0)
+		goto errout;
+	err = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETADDR,
+				   inet6_rtm_getaddr, inet6_dump_ifaddr,
+				   RTNL_FLAG_DOIT_UNLOCKED);
+	if (err < 0)
+		goto errout;
+	err = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETMULTICAST,
+				   NULL, inet6_dump_ifmcaddr, 0);
+	if (err < 0)
+		goto errout;
+	err = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETANYCAST,
+				   NULL, inet6_dump_ifacaddr, 0);
+	if (err < 0)
+		goto errout;
+	err = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETNETCONF,
+				   inet6_netconf_get_devconf,
+				   inet6_netconf_dump_devconf,
+				   RTNL_FLAG_DOIT_UNLOCKED);
+	if (err < 0)
+		goto errout;
 	ipv6_addr_label_rtnl_register();
 
 	return 0;
 errout:
+	rtnl_unregister_all(PF_INET6);
 	rtnl_af_unregister(&inet6_ops);
 	unregister_netdevice_notifier(&ipv6_dev_notf);
 errlo:
diff --git a/net/ipv6/addrlabel.c b/net/ipv6/addrlabel.c
index 00e1f8ee08f8..303fcce5beef 100644
--- a/net/ipv6/addrlabel.c
+++ b/net/ipv6/addrlabel.c
@@ -549,11 +549,10 @@ static int ip6addrlbl_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 
 void __init ipv6_addr_label_rtnl_register(void)
 {
-	__rtnl_register(PF_INET6, RTM_NEWADDRLABEL, ip6addrlbl_newdel,
-			NULL, RTNL_FLAG_DOIT_UNLOCKED);
-	__rtnl_register(PF_INET6, RTM_DELADDRLABEL, ip6addrlbl_newdel,
-			NULL, RTNL_FLAG_DOIT_UNLOCKED);
-	__rtnl_register(PF_INET6, RTM_GETADDRLABEL, ip6addrlbl_get,
-			ip6addrlbl_dump, RTNL_FLAG_DOIT_UNLOCKED);
+	rtnl_register(PF_INET6, RTM_NEWADDRLABEL, ip6addrlbl_newdel,
+		      NULL, RTNL_FLAG_DOIT_UNLOCKED);
+	rtnl_register(PF_INET6, RTM_DELADDRLABEL, ip6addrlbl_newdel,
+		      NULL, RTNL_FLAG_DOIT_UNLOCKED);
+	rtnl_register(PF_INET6, RTM_GETADDRLABEL, ip6addrlbl_get,
+		      ip6addrlbl_dump, RTNL_FLAG_DOIT_UNLOCKED);
 }
-
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index c43cbaedfa35..a64d559fa513 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -2142,8 +2142,8 @@ int __init fib6_init(void)
 	if (ret)
 		goto out_kmem_cache_create;
 
-	ret = __rtnl_register(PF_INET6, RTM_GETROUTE, NULL, inet6_dump_fib,
-			      0);
+	ret = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETROUTE, NULL,
+				   inet6_dump_fib, 0);
 	if (ret)
 		goto out_unregister_subsys;
 
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 46fd53b268da..b3f4d19b3ca5 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4772,11 +4772,20 @@ int __init ip6_route_init(void)
 	if (ret)
 		goto fib6_rules_init;
 
-	ret = -ENOBUFS;
-	if (__rtnl_register(PF_INET6, RTM_NEWROUTE, inet6_rtm_newroute, NULL, 0) ||
-	    __rtnl_register(PF_INET6, RTM_DELROUTE, inet6_rtm_delroute, NULL, 0) ||
-	    __rtnl_register(PF_INET6, RTM_GETROUTE, inet6_rtm_getroute, NULL,
-			    RTNL_FLAG_DOIT_UNLOCKED))
+	ret = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_NEWROUTE,
+				   inet6_rtm_newroute, NULL, 0);
+	if (ret < 0)
+		goto out_register_late_subsys;
+
+	ret = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_DELROUTE,
+				   inet6_rtm_delroute, NULL, 0);
+	if (ret < 0)
+		goto out_register_late_subsys;
+
+	ret = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETROUTE,
+				   inet6_rtm_getroute, NULL,
+				   RTNL_FLAG_DOIT_UNLOCKED);
+	if (ret < 0)
 		goto out_register_late_subsys;
 
 	ret = register_netdevice_notifier(&ip6_route_dev_notifier);
@@ -4794,6 +4803,7 @@ int __init ip6_route_init(void)
 	return ret;
 
 out_register_late_subsys:
+	rtnl_unregister_all(PF_INET6);
 	unregister_pernet_subsys(&ip6_route_net_late_ops);
 fib6_rules_init:
 	fib6_rules_cleanup();
-- 
2.13.6

^ permalink raw reply related

* [PATCH net-next 3/4] net: use rtnl_register_module where needed
From: Florian Westphal @ 2017-12-02 20:44 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal
In-Reply-To: <20171202204408.4166-1-fw@strlen.de>

all of these can be compiled as a module, so use new
_module version to make sure module can no longer be removed
while callback/dump is in use.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/bridge/br_mdb.c     |  6 +++---
 net/can/gw.c            | 14 ++++++++++----
 net/decnet/dn_dev.c     |  9 ++++++---
 net/decnet/dn_fib.c     |  6 ++++--
 net/decnet/dn_route.c   |  8 ++++----
 net/mpls/af_mpls.c      | 15 +++++++++------
 net/phonet/pn_netlink.c | 21 +++++++++++++--------
 net/qrtr/qrtr.c         |  8 ++++++--
 8 files changed, 55 insertions(+), 32 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index b0f4c734900b..6d9f48bd374a 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -760,9 +760,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 void br_mdb_init(void)
 {
-	rtnl_register(PF_BRIDGE, RTM_GETMDB, NULL, br_mdb_dump, 0);
-	rtnl_register(PF_BRIDGE, RTM_NEWMDB, br_mdb_add, NULL, 0);
-	rtnl_register(PF_BRIDGE, RTM_DELMDB, br_mdb_del, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_GETMDB, NULL, br_mdb_dump, 0);
+	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_NEWMDB, br_mdb_add, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_DELMDB, br_mdb_del, NULL, 0);
 }
 
 void br_mdb_uninit(void)
diff --git a/net/can/gw.c b/net/can/gw.c
index 73a02af4b5d7..398dd0395ad9 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -1014,6 +1014,8 @@ static struct pernet_operations cangw_pernet_ops = {
 
 static __init int cgw_module_init(void)
 {
+	int ret;
+
 	/* sanitize given module parameter */
 	max_hops = clamp_t(unsigned int, max_hops, CGW_MIN_HOPS, CGW_MAX_HOPS);
 
@@ -1031,15 +1033,19 @@ static __init int cgw_module_init(void)
 	notifier.notifier_call = cgw_notifier;
 	register_netdevice_notifier(&notifier);
 
-	if (__rtnl_register(PF_CAN, RTM_GETROUTE, NULL, cgw_dump_jobs, 0)) {
+	ret = rtnl_register_module(THIS_MODULE, PF_CAN, RTM_GETROUTE,
+				   NULL, cgw_dump_jobs, 0);
+	if (ret) {
 		unregister_netdevice_notifier(&notifier);
 		kmem_cache_destroy(cgw_cache);
 		return -ENOBUFS;
 	}
 
-	/* Only the first call to __rtnl_register can fail */
-	__rtnl_register(PF_CAN, RTM_NEWROUTE, cgw_create_job, NULL, 0);
-	__rtnl_register(PF_CAN, RTM_DELROUTE, cgw_remove_job, NULL, 0);
+	/* Only the first call to rtnl_register_module can fail */
+	rtnl_register_module(THIS_MODULE, PF_CAN, RTM_NEWROUTE,
+			     cgw_create_job, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_CAN, RTM_DELROUTE,
+			     cgw_remove_job, NULL, 0);
 
 	return 0;
 }
diff --git a/net/decnet/dn_dev.c b/net/decnet/dn_dev.c
index 9153247dad28..d1885cf59319 100644
--- a/net/decnet/dn_dev.c
+++ b/net/decnet/dn_dev.c
@@ -1418,9 +1418,12 @@ void __init dn_dev_init(void)
 
 	dn_dev_devices_on();
 
-	rtnl_register(PF_DECnet, RTM_NEWADDR, dn_nl_newaddr, NULL, 0);
-	rtnl_register(PF_DECnet, RTM_DELADDR, dn_nl_deladdr, NULL, 0);
-	rtnl_register(PF_DECnet, RTM_GETADDR, NULL, dn_nl_dump_ifaddr, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_NEWADDR,
+			     dn_nl_newaddr, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_DELADDR,
+			     dn_nl_deladdr, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_GETADDR,
+			     NULL, dn_nl_dump_ifaddr, 0);
 
 	proc_create("decnet_dev", S_IRUGO, init_net.proc_net, &dn_dev_seq_fops);
 
diff --git a/net/decnet/dn_fib.c b/net/decnet/dn_fib.c
index b37a1b833c77..fce94cbd4378 100644
--- a/net/decnet/dn_fib.c
+++ b/net/decnet/dn_fib.c
@@ -792,8 +792,10 @@ void __init dn_fib_init(void)
 
 	register_dnaddr_notifier(&dn_fib_dnaddr_notifier);
 
-	rtnl_register(PF_DECnet, RTM_NEWROUTE, dn_fib_rtm_newroute, NULL, 0);
-	rtnl_register(PF_DECnet, RTM_DELROUTE, dn_fib_rtm_delroute, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_NEWROUTE,
+			     dn_fib_rtm_newroute, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_DELROUTE,
+			     dn_fib_rtm_delroute, NULL, 0);
 }
 
 
diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index 4b3ca70be723..73160d4aebbe 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -1923,11 +1923,11 @@ void __init dn_route_init(void)
 		    &dn_rt_cache_seq_fops);
 
 #ifdef CONFIG_DECNET_ROUTER
-	rtnl_register(PF_DECnet, RTM_GETROUTE, dn_cache_getroute,
-		      dn_fib_dump, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_GETROUTE,
+			     dn_cache_getroute, dn_fib_dump, 0);
 #else
-	rtnl_register(PF_DECnet, RTM_GETROUTE, dn_cache_getroute,
-		      dn_cache_dump, 0);
+	rtnl_register_module(THIS_MODULE, PF_DECnet, RTM_GETROUTE,
+			     dn_cache_getroute, dn_cache_dump, 0);
 #endif
 }
 
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 8ca9915befc8..5dce8336d33f 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -2510,12 +2510,15 @@ static int __init mpls_init(void)
 
 	rtnl_af_register(&mpls_af_ops);
 
-	rtnl_register(PF_MPLS, RTM_NEWROUTE, mpls_rtm_newroute, NULL, 0);
-	rtnl_register(PF_MPLS, RTM_DELROUTE, mpls_rtm_delroute, NULL, 0);
-	rtnl_register(PF_MPLS, RTM_GETROUTE, mpls_getroute, mpls_dump_routes,
-		      0);
-	rtnl_register(PF_MPLS, RTM_GETNETCONF, mpls_netconf_get_devconf,
-		      mpls_netconf_dump_devconf, 0);
+	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_NEWROUTE,
+			     mpls_rtm_newroute, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_DELROUTE,
+			     mpls_rtm_delroute, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_GETROUTE,
+			     mpls_getroute, mpls_dump_routes, 0);
+	rtnl_register_module(THIS_MODULE, PF_MPLS, RTM_GETNETCONF,
+			     mpls_netconf_get_devconf,
+			     mpls_netconf_dump_devconf, 0);
 	err = ipgre_tunnel_encap_add_mpls_ops();
 	if (err)
 		pr_err("Can't add mpls over gre tunnel ops\n");
diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
index da754fc926e7..871eaf2cb85e 100644
--- a/net/phonet/pn_netlink.c
+++ b/net/phonet/pn_netlink.c
@@ -299,16 +299,21 @@ static int route_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 
 int __init phonet_netlink_register(void)
 {
-	int err = __rtnl_register(PF_PHONET, RTM_NEWADDR, addr_doit,
-				  NULL, 0);
+	int err = rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_NEWADDR,
+				       addr_doit, NULL, 0);
 	if (err)
 		return err;
 
-	/* Further __rtnl_register() cannot fail */
-	__rtnl_register(PF_PHONET, RTM_DELADDR, addr_doit, NULL, 0);
-	__rtnl_register(PF_PHONET, RTM_GETADDR, NULL, getaddr_dumpit, 0);
-	__rtnl_register(PF_PHONET, RTM_NEWROUTE, route_doit, NULL, 0);
-	__rtnl_register(PF_PHONET, RTM_DELROUTE, route_doit, NULL, 0);
-	__rtnl_register(PF_PHONET, RTM_GETROUTE, NULL, route_dumpit, 0);
+	/* Further rtnl_register_module() cannot fail */
+	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_DELADDR,
+			     addr_doit, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_GETADDR,
+			     NULL, getaddr_dumpit, 0);
+	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_NEWROUTE,
+			     route_doit, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_DELROUTE,
+			     route_doit, NULL, 0);
+	rtnl_register_module(THIS_MODULE, PF_PHONET, RTM_GETROUTE,
+			     NULL, route_dumpit, 0);
 	return 0;
 }
diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index 77ab05e23001..5fb3929e3d7d 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -1116,9 +1116,13 @@ static int __init qrtr_proto_init(void)
 		return rc;
 	}
 
-	rtnl_register(PF_QIPCRTR, RTM_NEWADDR, qrtr_addr_doit, NULL, 0);
+	rc = rtnl_register_module(THIS_MODULE, PF_QIPCRTR, RTM_NEWADDR, qrtr_addr_doit, NULL, 0);
+	if (rc) {
+		sock_unregister(qrtr_family.family);
+		proto_unregister(&qrtr_proto);
+	}
 
-	return 0;
+	return rc;
 }
 postcore_initcall(qrtr_proto_init);
 
-- 
2.13.6

^ permalink raw reply related

* [PATCH net-next 2/4] rtnetlink: get reference on module before invoking handlers
From: Florian Westphal @ 2017-12-02 20:44 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal, Peter Zijlstra
In-Reply-To: <20171202204408.4166-1-fw@strlen.de>

Add yet another rtnl_register function.  It will be used by modules
that can be removed.

The passed module struct is used to prevent module unload while
a netlink dump is in progress or when a DOIT_UNLOCKED doit callback
is called.

Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/rtnetlink.h |   2 +
 net/core/rtnetlink.c    | 113 +++++++++++++++++++++++++++++++++---------------
 2 files changed, 80 insertions(+), 35 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index ead018744ff5..e326b3f9eb5f 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -17,6 +17,8 @@ int __rtnl_register(int protocol, int msgtype,
 		    rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
+int rtnl_register_module(struct module *owner, int protocol, int msgtype,
+			 rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 int rtnl_unregister(int protocol, int msgtype);
 void rtnl_unregister_all(int protocol);
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index ff292d3f2c41..de6390365c90 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -62,6 +62,7 @@
 struct rtnl_link {
 	rtnl_doit_func		doit;
 	rtnl_dumpit_func	dumpit;
+	struct module		*owner;
 	unsigned int		flags;
 	struct rcu_head		rcu;
 };
@@ -129,7 +130,6 @@ EXPORT_SYMBOL(lockdep_rtnl_is_held);
 #endif /* #ifdef CONFIG_PROVE_LOCKING */
 
 static struct rtnl_link __rcu **rtnl_msg_handlers[RTNL_FAMILY_MAX + 1];
-static refcount_t rtnl_msg_handlers_ref[RTNL_FAMILY_MAX + 1];
 
 static inline int rtm_msgindex(int msgtype)
 {
@@ -159,27 +159,10 @@ static struct rtnl_link *rtnl_get_link(int protocol, int msgtype)
 	return tab[msgtype];
 }
 
-/**
- * __rtnl_register - Register a rtnetlink message type
- * @protocol: Protocol family or PF_UNSPEC
- * @msgtype: rtnetlink message type
- * @doit: Function pointer called for each request message
- * @dumpit: Function pointer called for each dump request (NLM_F_DUMP) message
- * @flags: rtnl_link_flags to modifiy behaviour of doit/dumpit functions
- *
- * Registers the specified function pointers (at least one of them has
- * to be non-NULL) to be called whenever a request message for the
- * specified protocol family and message type is received.
- *
- * The special protocol family PF_UNSPEC may be used to define fallback
- * function pointers for the case when no entry for the specific protocol
- * family exists.
- *
- * Returns 0 on success or a negative error code.
- */
-int __rtnl_register(int protocol, int msgtype,
-		    rtnl_doit_func doit, rtnl_dumpit_func dumpit,
-		    unsigned int flags)
+static int rtnl_register_internal(struct module *owner,
+				  int protocol, int msgtype,
+				  rtnl_doit_func doit, rtnl_dumpit_func dumpit,
+				  unsigned int flags)
 {
 	struct rtnl_link **tab, *link, *old;
 	int msgindex;
@@ -210,6 +193,9 @@ int __rtnl_register(int protocol, int msgtype,
 			goto unlock;
 	}
 
+	WARN_ON(link->owner && link->owner != owner);
+	link->owner = owner;
+
 	WARN_ON(doit && link->doit && link->doit != doit);
 	if (doit)
 		link->doit = doit;
@@ -228,6 +214,54 @@ int __rtnl_register(int protocol, int msgtype,
 	rtnl_unlock();
 	return ret;
 }
+
+/**
+ * rtnl_register_module - Register a rtnetlink message type
+ *
+ * @owner: module registering the hook (THIS_MODULE)
+ * @protocol: Protocol family or PF_UNSPEC
+ * @msgtype: rtnetlink message type
+ * @doit: Function pointer called for each request message
+ * @dumpit: Function pointer called for each dump request (NLM_F_DUMP) message
+ * @flags: rtnl_link_flags to modifiy behaviour of doit/dumpit functions
+ *
+ * Like rtnl_register, but for use by removable modules.
+ */
+int rtnl_register_module(struct module *owner,
+			 int protocol, int msgtype,
+			 rtnl_doit_func doit, rtnl_dumpit_func dumpit,
+			 unsigned int flags)
+{
+	return rtnl_register_internal(owner, protocol, msgtype,
+				      doit, dumpit, flags);
+}
+EXPORT_SYMBOL_GPL(rtnl_register_module);
+
+/**
+ * __rtnl_register - Register a rtnetlink message type
+ * @protocol: Protocol family or PF_UNSPEC
+ * @msgtype: rtnetlink message type
+ * @doit: Function pointer called for each request message
+ * @dumpit: Function pointer called for each dump request (NLM_F_DUMP) message
+ * @flags: rtnl_link_flags to modifiy behaviour of doit/dumpit functions
+ *
+ * Registers the specified function pointers (at least one of them has
+ * to be non-NULL) to be called whenever a request message for the
+ * specified protocol family and message type is received.
+ *
+ * The special protocol family PF_UNSPEC may be used to define fallback
+ * function pointers for the case when no entry for the specific protocol
+ * family exists.
+ *
+ * Returns 0 on success or a negative error code.
+ */
+int __rtnl_register(int protocol, int msgtype,
+		    rtnl_doit_func doit, rtnl_dumpit_func dumpit,
+		    unsigned int flags)
+{
+	return rtnl_register_internal(NULL, protocol, msgtype,
+				      doit, dumpit, flags);
+}
 EXPORT_SYMBOL_GPL(__rtnl_register);
 
 /**
@@ -311,8 +345,6 @@ void rtnl_unregister_all(int protocol)
 
 	synchronize_net();
 
-	while (refcount_read(&rtnl_msg_handlers_ref[protocol]) > 1)
-		schedule();
 	kfree(tab);
 }
 EXPORT_SYMBOL_GPL(rtnl_unregister_all);
@@ -4372,6 +4404,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 {
 	struct net *net = sock_net(skb->sk);
 	struct rtnl_link *link;
+	struct module *owner;
 	int err = -EOPNOTSUPP;
 	rtnl_doit_func doit;
 	unsigned int flags;
@@ -4408,24 +4441,32 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 			if (!link || !link->dumpit)
 				goto err_unlock;
 		}
+		owner = link->owner;
 		dumpit = link->dumpit;
 
-		refcount_inc(&rtnl_msg_handlers_ref[family]);
-
 		if (type == RTM_GETLINK - RTM_BASE)
 			min_dump_alloc = rtnl_calcit(skb, nlh);
 
+		err = 0;
+		/* need to do this before rcu_read_unlock() */
+		if (!try_module_get(owner))
+			err = -EPROTONOSUPPORT;
+
 		rcu_read_unlock();
 
 		rtnl = net->rtnl;
-		{
+		if (err == 0) {
 			struct netlink_dump_control c = {
 				.dump		= dumpit,
 				.min_dump_alloc	= min_dump_alloc,
+				.module		= owner,
 			};
 			err = netlink_dump_start(rtnl, skb, nlh, &c);
+			/* netlink_dump_start() will keep a reference on
+			 * module if dump is still in progress.
+			 */
+			module_put(owner);
 		}
-		refcount_dec(&rtnl_msg_handlers_ref[family]);
 		return err;
 	}
 
@@ -4437,14 +4478,19 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 			goto out_unlock;
 	}
 
+	owner = link->owner;
+	if (!try_module_get(owner)) {
+		err = -EPROTONOSUPPORT;
+		goto out_unlock;
+	}
+
 	flags = link->flags;
 	if (flags & RTNL_FLAG_DOIT_UNLOCKED) {
-		refcount_inc(&rtnl_msg_handlers_ref[family]);
 		doit = link->doit;
 		rcu_read_unlock();
 		if (doit)
 			err = doit(skb, nlh, extack);
-		refcount_dec(&rtnl_msg_handlers_ref[family]);
+		module_put(owner);
 		return err;
 	}
 	rcu_read_unlock();
@@ -4455,6 +4501,8 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 		err = link->doit(skb, nlh, extack);
 	rtnl_unlock();
 
+	module_put(owner);
+
 	return err;
 
 out_unlock:
@@ -4546,11 +4594,6 @@ static struct pernet_operations rtnetlink_net_ops = {
 
 void __init rtnetlink_init(void)
 {
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(rtnl_msg_handlers_ref); i++)
-		refcount_set(&rtnl_msg_handlers_ref[i], 1);
-
 	if (register_pernet_subsys(&rtnetlink_net_ops))
 		panic("rtnetlink_init: cannot initialize rtnetlink\n");
 
-- 
2.13.6

^ 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