* Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
From: Jason A. Donenfeld @ 2018-09-29 2:20 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson, Russell King - ARM Linux,
linux-arm-kernel
In-Reply-To: <CAKv+Gu8knzzmK4_KouFQWKzMdHFgEsk-CtBrcuAvk0bYkDFO=w@mail.gmail.com>
Hi Ard,
On Fri, Sep 28, 2018 at 6:02 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> Please put comments like this below the ---
git-notes is nice for this indeed.
> Are these CONFIG_ symbols defined anywhere at this point?
Yes, they're introduced in the first zinc commit. There's no git-blame
on git.kernel.org, presumably because it's expensive to compute, but
there is on my personal instance, so this might help:
https://git.zx2c4.com/linux-dev/blame/lib/zinc/Kconfig?h=jd/wireguard
> In any case, I don't think these is a reason for these, at least not
> on ARM/arm64. The 64-bitness is implied in both cases
You mean to say that since these nobs are def_bool y and are
essentially "depends on ARM", then I should just straight up use
CONFIG_ARM? I had thought about this, but figured this would make it
easier to later make these optional or have other options block them
need be, or even if the dependencies and requirements for having them
changes (for example, with UML on x86). I think doing it this way
gives us some flexibility later on. So if that's a compelling enough
reason, I'd like to keep those.
> and the
> dependency on !CPU_32v3 you introduce (looking at the version of
> Kconfig at the end of the series) seems spurious to me. Was that added
> because of some kbuild robot report? (we don't support ARMv3 in the
> kernel but ARCH_RPC is built in v3 mode because of historical reasons
> while the actual core is a v4)
I added the !CPU_32v3 in my development tree after posting v6, so good
to hear you're just looking straight at the updated tree. If you see
things jump out in there prior to me posting v7, don't hesitate to let
me know.
The reason it was added was indeed because of:
https://lists.01.org/pipermail/kbuild-all/2018-September/053114.html
-- exactly what you suspected, ARCH_RPC. Have a better suggestion than
!CPU_32v3? It seems to me like so long as the kernel has CPU_32v3 as a
thing in any form, I should mark Zinc as not supporting it, since
we'll certainly be at least v4 and up. (Do you guys have any old Acorn
ARM610 boxes sitting around for old time's sake at LinaroHQ? ;-)
> > +#endif
> > +
>
> No need to make asmlinkage declarations conditional
Yep, addressed in the IS_ENABLED cleanup.
>
> if (IS_ENABLED())
Sorted.
>
> """
> if (!IS_ENABLED(CONFIG_ARM))
> return false;
>
> hchacha20_arm(x, derived_key);
> return true;
> """
>
> and drop the #ifdefs
Also sorted.
Regards,
Jason
^ permalink raw reply
* Re: bond: take rcu lock in bond_poll_controller
From: Dave Jones @ 2018-09-28 19:49 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAM_iQpUdoyhKKYOgYtMBv__ZX81R9N2Ox-mftP5mTbgb1h=kfg@mail.gmail.com>
On Fri, Sep 28, 2018 at 12:03:22PM -0700, Cong Wang wrote:
> On Fri, Sep 28, 2018 at 12:02 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Fri, Sep 28, 2018 at 11:26 AM Dave Jones <davej@codemonkey.org.uk> wrote:
> > > diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> > > index 3219a2932463..4f9494381635 100644
> > > --- a/net/core/netpoll.c
> > > +++ b/net/core/netpoll.c
> > > @@ -330,6 +330,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
> > > /* It is up to the caller to keep npinfo alive. */
> > > struct netpoll_info *npinfo;
> > >
> > > + rcu_read_lock();
> > > lockdep_assert_irqs_disabled();
> > >
> > > npinfo = rcu_dereference_bh(np->dev->npinfo);
> >
> > I think you probably need rcu_read_lock_bh() to satisfy
> > rcu_deference_bh()...
>
> But irq is disabled here, so not sure if rcu_read_lock_bh()
> could cause trouble... Interesting...
I was wondering for a moment why I never got a warning, then I
remembered I disabled lockdep for that machine because nfs spews stuff.
I'll doublecheck, and post v4. lol, this looked like a 2 minute fix at first.
Dave
^ permalink raw reply
* [net-next:master 729/738] drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c:496:31: error: 'HNAE3_REVISION_ID_21' undeclared; did you mean 'FADT2_REVISION_ID'?
From: kbuild test robot @ 2018-09-28 19:43 UTC (permalink / raw)
To: Fuyun Liang; +Cc: kbuild-all, netdev, Peng Li, Salil Mehta
[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git master
head: 5362700c942b2cc4bab328361545a6d6fe649534
commit: 4dc13b9668d8ba7a5d1a26b88fa30baa8a214dcc [729/738] net: hns3: Add serdes parallel inner loopback support
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout 4dc13b9668d8ba7a5d1a26b88fa30baa8a214dcc
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c: In function 'hclge_get_sset_count':
>> drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c:496:31: error: 'HNAE3_REVISION_ID_21' undeclared (first use in this function); did you mean 'FADT2_REVISION_ID'?
if (hdev->pdev->revision >= HNAE3_REVISION_ID_21 ||
^~~~~~~~~~~~~~~~~~~~
FADT2_REVISION_ID
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c:496:31: note: each undeclared identifier is reported only once for each function it appears in
vim +496 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
476
477 static int hclge_get_sset_count(struct hnae3_handle *handle, int stringset)
478 {
479 #define HCLGE_LOOPBACK_TEST_FLAGS (HNAE3_SUPPORT_APP_LOOPBACK |\
480 HNAE3_SUPPORT_PHY_LOOPBACK |\
481 HNAE3_SUPPORT_SERDES_SERIAL_LOOPBACK |\
482 HNAE3_SUPPORT_SERDES_PARALLEL_LOOPBACK)
483
484 struct hclge_vport *vport = hclge_get_vport(handle);
485 struct hclge_dev *hdev = vport->back;
486 int count = 0;
487
488 /* Loopback test support rules:
489 * mac: only GE mode support
490 * serdes: all mac mode will support include GE/XGE/LGE/CGE
491 * phy: only support when phy device exist on board
492 */
493 if (stringset == ETH_SS_TEST) {
494 /* clear loopback bit flags at first */
495 handle->flags = (handle->flags & (~HCLGE_LOOPBACK_TEST_FLAGS));
> 496 if (hdev->pdev->revision >= HNAE3_REVISION_ID_21 ||
497 hdev->hw.mac.speed == HCLGE_MAC_SPEED_10M ||
498 hdev->hw.mac.speed == HCLGE_MAC_SPEED_100M ||
499 hdev->hw.mac.speed == HCLGE_MAC_SPEED_1G) {
500 count += 1;
501 handle->flags |= HNAE3_SUPPORT_APP_LOOPBACK;
502 }
503
504 count += 2;
505 handle->flags |= HNAE3_SUPPORT_SERDES_SERIAL_LOOPBACK;
506 handle->flags |= HNAE3_SUPPORT_SERDES_PARALLEL_LOOPBACK;
507 } else if (stringset == ETH_SS_STATS) {
508 count = ARRAY_SIZE(g_mac_stats_string) +
509 hclge_tqps_get_sset_count(handle, stringset);
510 }
511
512 return count;
513 }
514
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65237 bytes --]
^ permalink raw reply
* Re: [PATCH net v2 0/2] net: phy: fix WoL handling when suspending the PHY
From: Heiner Kallweit @ 2018-09-28 19:41 UTC (permalink / raw)
To: David Miller; +Cc: andrew, f.fainelli, nic_swsd, netdev
In-Reply-To: <20180926.200435.34563447557737934.davem@davemloft.net>
On 27.09.2018 05:04, David Miller wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Mon, 24 Sep 2018 21:58:04 +0200
>
>> phy_suspend doesn't always recognize that WoL is enabled and therefore
>> suspends the PHY when it should not. First idea to address the issue
>> was to reuse checks used in mdio_bus_phy_may_suspend which check
>> whether relevant devices are wakeup-enabled.
>> Florian raised some concerns because drivers may enable wakeup even if
>> WoL isn't enabled (e.g. certain USB network drivers).
>>
>> The new approach focuses on reducing the risk to break existing stuff.
>> We add a flag wol_enabled to struct net_device which is set in
>> ethtool_set_wol(). Then this flag is checked in phy_suspend().
>> This doesn't cover 100% of the cases yet (e.g. if WoL is enabled w/o
>> explicit configuration), but it covers the most relevant cases with
>> very little risk of regressions.
>>
>> v2:
>> - Fix a typo
>
> Series applied, thanks.
>
> Please deal with the extra 4 byte size of net_device in net-next.
>
Sure. Next week I'm on travel, will do it the week after.
> Thanks.
>
^ permalink raw reply
* Re: [PATCH net-next v6 04/23] zinc: ChaCha20 x86_64 implementation
From: Jason A. Donenfeld @ 2018-09-29 2:01 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson, Andy Polyakov, Thomas Gleixner, mingo,
X86 ML
In-Reply-To: <CAKv+Gu9aOiYdEzHHrHFHUFK=hNfeS=zEsvMe9OWD8hEdAYkffg@mail.gmail.com>
Hi Ard,
On Fri, Sep 28, 2018 at 5:47 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> Please drop this SOB line: SOB is not about (co-)authorship but about
> who handled the patch on its way into mainline.
The s-o-b line is about the DCoO, which is given here by both of us,
with the code starting in a shared tree that only had that s-o-b. I
can remove it though if you think it's strange to have two s-o-bs.
> You are sending the
> patch so your SOB should come last.
I was wondering about the ordering of these, actually. I've seen s-o-b
on top and s-o-b on bottom of the cc list in lots of commits and
haven't yet divined the One True Position.
> You can drop the #ifdefs above ...
Yep. All of those ifdefs in this and in the other primitives are
already gone, and everything is fully IS_ENABLEDenabled now.
> > +#ifdef CONFIG_AS_AVX512
>
> ... and use IS_ENABLED(CONFIG_AS_AVX512) here inside the if().
Already done, per your suggestion a few days ago.
> > -#ifndef HAVE_CHACHA20_ARCH_IMPLEMENTATION
>
> As I mentioned in reply to the previous patch, please get rid of this
> CPP symbol ^^^
Ack.
Jason
^ permalink raw reply
* [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers
From: Mauricio Faria de Oliveira @ 2018-09-28 19:35 UTC (permalink / raw)
To: netdev; +Cc: davem
Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg',
which is not always true. For example, 'struct ndmsg' is used by iproute2
as well (in the 'ip neigh' command).
The problem is, the function bails out early if nlmsg_parse() fails, which
does occur for iproute2 usage of 'struct ndmsg' because the payload length
is shorter than the family header alone (as 'struct ifinfomsg' is assumed).
This breaks backward compatibility with userspace (different response) and
is a regression due to commit 0ff50e83b512 ("net: rtnetlink: bail out from
rtnl_fdb_dump() on parse error").
Some examples with iproute2 and netlink library for go [1]:
1) $ bridge fdb show
33:33:00:00:00:01 dev ens3 self permanent
01:00:5e:00:00:01 dev ens3 self permanent
33:33:ff:15:98:30 dev ens3 self permanent
This one works, as it uses 'struct ifinfomsg'.
fdb_show() @ iproute2/bridge/fdb.c
"""
.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
...
if (rtnl_dump_request(&rth, RTM_GETNEIGH, [...]
"""
2) $ ip --family bridge neigh
RTNETLINK answers: Invalid argument
Dump terminated
This one fails, as it uses 'struct ndmsg'.
do_show_or_flush() @ iproute2/ip/ipneigh.c
"""
.n.nlmsg_type = RTM_GETNEIGH,
.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ndmsg)),
"""
3) $ ./neighlist
< no output >
This one fails, as it uses 'struct ndmsg'-based.
neighList() @ netlink/neigh_linux.go
"""
req := h.newNetlinkRequest(unix.RTM_GETNEIGH, [...]
msg := Ndmsg{
"""
The actual breakage was introduced by commit 0ff50e83b512 ("net: rtnetlink:
bail out from rtnl_fdb_dump() on parse error"), because nlmsg_parse() fails
if the payload length (with the _actual_ family header) is less than the
family header length alone (which is assumed, in parameter 'hdrlen').
This is true in the examples above with struct ndmsg, with size and payload
length shorter than struct ifinfomsg.
However, that commit just intends to fix something under the assumption the
family header is indeed an 'struct ifinfomsg' - by preventing access to the
payload as such (via 'ifm' pointer) if the payload length is not sufficient
to actually contain it.
The assumption was introduced by commit 5e6d24358799 ("bridge: netlink dump
interface at par with brctl"), to support iproute2's 'bridge fdb' command
(not 'ip neigh') which indeed uses 'struct ifinfomsg', thus is not broken.
So, in order to unbreak shorter family headers as 'struct ndmsg' and still
prevent access to invalid payload data, let's revert that former fix, and
move ifinfomsg payload access (via ifm) into nlmsg_parse() successful case
(i.e., the payload length is sufficient to be accessed as struct ifinfomsg)
which also works/returns 0 even in case there are no attributes in payload.
Same examples with this patch applied (or revert/before the original fix):
$ bridge fdb show
33:33:00:00:00:01 dev ens3 self permanent
01:00:5e:00:00:01 dev ens3 self permanent
33:33:ff:15:98:30 dev ens3 self permanent
$ ip --family bridge neigh
dev ens3 lladdr 33:33:00:00:00:01 PERMANENT
dev ens3 lladdr 01:00:5e:00:00:01 PERMANENT
dev ens3 lladdr 33:33:ff:15:98:30 PERMANENT
$ ./neighlist
netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0x0, 0x0, 0x0, 0x1}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0}
netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x1, 0x0, 0x5e, 0x0, 0x0, 0x1}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0}
netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0xff, 0x15, 0x98, 0x30}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0}
Tested on mainline (ad0371482b1e) and net-next (a804e5e21875) on Sep. 28.
References:
[1] netlink library for go (test-case)
https://github.com/vishvananda/netlink
$ cat ~/go/src/neighlist/main.go
package main
import ("fmt"; "syscall"; "github.com/vishvananda/netlink")
func main() {
neighs, _ := netlink.NeighList(0, syscall.AF_BRIDGE)
for _, neigh := range neighs { fmt.Printf("%#v\n", neigh) }
}
$ export GOPATH=~/go
$ go get github.com/vishvananda/netlink
$ go build neighlist
$ ~/go/src/neighlist/neighlist
Fixes: 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse error")
Fixes: 5e6d24358799 ("bridge: netlink dump interface at par with brctl")
Reported-by: Aidan Obley <aobley@pivotal.io>
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
---
P.S.: this may be 'net', but labeling as 'net-next' for possible relation to recent thread
[PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data in dump request
net/core/rtnetlink.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 60c928894a78..9695a27cc9b9 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3744,16 +3744,17 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
int err = 0;
int fidx = 0;
- err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
- IFLA_MAX, ifla_policy, NULL);
- if (err < 0) {
- return -EINVAL;
- } else if (err == 0) {
+ /* The family header may _not_ be struct ifinfomsg
+ * (e.g., struct ndmsg). Usage of the ifm pointer
+ * must check payload length (e.g., nlmsg_parse()).
+ */
+ if (nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
+ IFLA_MAX, ifla_policy, NULL) == 0) {
if (tb[IFLA_MASTER])
br_idx = nla_get_u32(tb[IFLA_MASTER]);
- }
- brport_idx = ifm->ifi_index;
+ brport_idx = ifm->ifi_index;
+ }
if (br_idx) {
br_dev = __dev_get_by_index(net, br_idx);
--
2.17.1
^ permalink raw reply related
* [PATCH net] rtnetlink: Fail dump if target netnsid is invalid
From: David Ahern @ 2018-09-28 19:28 UTC (permalink / raw)
To: netdev; +Cc: jbenc, davem, David Ahern
From: David Ahern <dsahern@gmail.com>
Link dumps can return results from a target namespace. If the namespace id
is invalid, then the dump request should fail if get_target_net fails
rather than continuing with a dump of the current namespace.
Fixes: 79e1ad148c844 ("rtnetlink: use netnsid to query interface")
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/core/rtnetlink.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 63ce2283a456..7f37fe9c65a5 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1898,10 +1898,8 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
if (tb[IFLA_IF_NETNSID]) {
netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
tgt_net = get_target_net(skb->sk, netnsid);
- if (IS_ERR(tgt_net)) {
- tgt_net = net;
- netnsid = -1;
- }
+ if (IS_ERR(tgt_net))
+ return PTR_ERR(tgt_net);
}
if (tb[IFLA_EXT_MASK])
--
2.11.0
^ permalink raw reply related
* Re: [PATCH net-next v6 03/23] zinc: ChaCha20 generic C implementation and selftest
From: Jason A. Donenfeld @ 2018-09-29 1:53 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <CAKv+Gu8dscmuEnRYNACFfCudwVkA6CRHhTe7K-=r_SfJQBGgzQ@mail.gmail.com>
Hi Ard,
On Fri, Sep 28, 2018 at 5:40 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> > +struct chacha20_ctx {
> > + u32 constant[4];
> > + u32 key[8];
> > + u32 counter[4];
> > +} __aligned(32);
> > +
>
> 32 *byte* alignment? Is that right? If this is for performance and it
> actually helps, using __cacheline_aligned is more appropriate,
It was originally this way, I believe, for vmovdqa, which requires
32-byte alignment in vex.256 encoding, and I never removed it after
changing some things. But I'll spend some time ensuring this is so and
if it doesn't make sense anymore it'll be gone by v7. On the other
hand, your suggestion of __cacheline_aligned may actually be something
worth considering, especially on MIPS.
> I guess this include is for crypto_xor_cpy() ?
>
> We may want to put a comment here, so we keep track of the interdependencies.
Right, it's for crypto_xor_cpy. Good idea, I'll add the comment.
> > +#ifndef HAVE_CHACHA20_ARCH_IMPLEMENTATION
>
> This #define is never set in subsequent patches, so just drop this
> #ifndef entirely (for this patch only)
Okay. It's also there for the other primitives too; I'll nix it for all of them.
> Return values from initcalls are ignored, and given that chacha20 will
> be depended upon by random.c, it will never be a module in practice.
>
> Given your previous statement that selftest should *not* be a DEBUG
> feature (which I wholeheartedly agree with), you could be a bit
> noisier here imo.
> E.g.,
>
> if (WARN_ON(!chach20_selftest())
> return ...
That's an excellent idea. We could bloat the whole thing with something like:
#ifdef MODULE
#define WARN_ON_IF_MODULE(x) (x)
#else
#define WARN_ON_IF_MODULE(x) WARN_ON(x)
#endif
But given that kind of thing would probably need to touch more files
in the tree, and hence involve more drawn-out conversation, I'll keep
it as the simple WARN_ON for now. Besides, being noisy no matter what
might actually be the best strategy for receiving bug reports on what
is potentially a pretty catastrophic error.
Thanks for the review.
Jason
^ permalink raw reply
* Re: bond: take rcu lock in bond_poll_controller
From: Cong Wang @ 2018-09-28 19:03 UTC (permalink / raw)
To: Dave Jones; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAM_iQpV=LdkNmgZMGkofHqr4gjFA0+FQKFO0gSxmXMAZ7zwX1g@mail.gmail.com>
On Fri, Sep 28, 2018 at 12:02 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Fri, Sep 28, 2018 at 11:26 AM Dave Jones <davej@codemonkey.org.uk> wrote:
> > diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> > index 3219a2932463..4f9494381635 100644
> > --- a/net/core/netpoll.c
> > +++ b/net/core/netpoll.c
> > @@ -330,6 +330,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
> > /* It is up to the caller to keep npinfo alive. */
> > struct netpoll_info *npinfo;
> >
> > + rcu_read_lock();
> > lockdep_assert_irqs_disabled();
> >
> > npinfo = rcu_dereference_bh(np->dev->npinfo);
>
> I think you probably need rcu_read_lock_bh() to satisfy
> rcu_deference_bh()...
But irq is disabled here, so not sure if rcu_read_lock_bh()
could cause trouble... Interesting...
^ permalink raw reply
* Re: bond: take rcu lock in bond_poll_controller
From: Cong Wang @ 2018-09-28 19:02 UTC (permalink / raw)
To: Dave Jones; +Cc: Linux Kernel Network Developers
In-Reply-To: <20180928182410.slizlkx6fiwkyb76@codemonkey.org.uk>
On Fri, Sep 28, 2018 at 11:26 AM Dave Jones <davej@codemonkey.org.uk> wrote:
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 3219a2932463..4f9494381635 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -330,6 +330,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
> /* It is up to the caller to keep npinfo alive. */
> struct netpoll_info *npinfo;
>
> + rcu_read_lock();
> lockdep_assert_irqs_disabled();
>
> npinfo = rcu_dereference_bh(np->dev->npinfo);
I think you probably need rcu_read_lock_bh() to satisfy
rcu_deference_bh()...
^ permalink raw reply
* Re: [PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data in dump request
From: Christian Brauner @ 2018-09-28 18:45 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20180928154502.9983-1-dsahern@kernel.org>
On Fri, Sep 28, 2018 at 08:44:57AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
>
> There are many use cases where a user wants to influence what is
> returned in a dump for some rtnetlink command: one is wanting data
> for a different namespace than the one the request is received and
> another is limiting the amount of data returned in the dump to a
> specific set of interest to userspace, reducing the cpu overhead of
> both kernel and userspace. Unfortunately, the kernel has historically
> not been strict with checking for the proper header or checking the
> values passed in the header. This lenient implementation has allowed
> iproute2 and other packages to pass any struct or data in the dump
> request as long as the family is the first byte. For example, ifinfomsg
> struct is used by iproute2 for all generic dump requests - links,
> addresses, routes and rules when it is really only valid for link
> requests.
>
> There is 1 is example where the kernel deals with the wrong struct: link
> dumps after VF support was added. Older iproute2 was sending rtgenmsg as
> the header instead of ifinfomsg so a patch was added to try and detect
> old userspace vs new:
> e5eca6d41f53 ("rtnetlink: fix userspace API breakage for iproute2 < v3.9.0")
>
> The latest example is Christian's patch set wanting to return addresses for
> a target namespace. It guesses the header struct is an ifaddrmsg and if it
> guesses wrong a netlink warning is generated in the kernel log on every
> address dump which is unacceptable.
>
> Another example where the kernel is a bit lenient is route dumps: iproute2
> can send either a request with either ifinfomsg or a rtmsg as the header
> struct, yet the kernel always treats the header as an rtmsg (see
> inet_dump_fib and rtm_flags check).
>
> How to resolve the problem of not breaking old userspace yet be able to
> move forward with new features such as kernel side filtering which are
> crucial for efficient operation at high scale?
>
> This patch set addresses the problem by adding a new netlink flag,
> NLM_F_DUMP_PROPER_HDR, that userspace can set to say "I have a clue, and
> I am sending the right header struct" and that the struct fields and any
> attributes after it should be used for filtering the data returned in the
> dump.
>
> Kernel side, the dump handlers are updated to check every field in the
> header struct and all attributes passed. Only ones where filtering is
> implemented are allowed to be set. Any other values cause the dump to fail
> with EINVAL. If the new flag is honored by the kernel and the dump contents
> adjusted by any data passed in the request, the dump handler sets the
> NLM_F_DUMP_FILTERED flag in the netlink message header.
>
> This is an RFC set with the address handlers updated. If the approach is
> acceptable, then I will do the same to the other rtnetlink dump handlers.
I like the idea and I think this might be a good solution to this
problem. If we can agree on this in favor of mine I'm all for it!
Thanks, David!
Christian
>
>
> David Ahern (5):
> net/netlink: Pass extack to dump callbacks
> net/ipv6: Refactor address dump to push inet6_fill_args to
> in6_dump_addrs
> netlink: introduce NLM_F_DUMP_PROPER_HDR flag
> net/ipv4: Update inet_dump_ifaddr to support NLM_F_DUMP_PROPER_HDR
> net/ipv6: Update inet6_dump_addr to support NLM_F_DUMP_PROPER_HDR
>
> include/linux/netlink.h | 2 +
> include/uapi/linux/netlink.h | 1 +
> net/core/rtnetlink.c | 1 +
> net/ipv4/devinet.c | 52 +++++++++++++++++-----
> net/ipv6/addrconf.c | 101 +++++++++++++++++++++++++++++--------------
> net/netlink/af_netlink.c | 1 +
> 6 files changed, 114 insertions(+), 44 deletions(-)
>
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH RFC net-next 2/5] net/ipv6: Refactor address dump to push inet6_fill_args to in6_dump_addrs
From: Christian Brauner @ 2018-09-28 18:44 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20180928154502.9983-3-dsahern@kernel.org>
On Fri, Sep 28, 2018 at 08:44:59AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Pull the inet6_fill_args arg up to in6_dump_addrs and move netnsid
> into it. Since IFA_TARGET_NETNSID is a kernel side filter add the
> NLM_F_DUMP_FILTERED flag so userspace knows the request was honored.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Christian Brauner <christian@brauner.io>
> ---
> net/ipv6/addrconf.c | 59 +++++++++++++++++++++++++++++------------------------
> 1 file changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index a9a317322388..375ea9d9869b 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4793,12 +4793,19 @@ static inline int inet6_ifaddr_msgsize(void)
> + nla_total_size(4) /* IFA_RT_PRIORITY */;
> }
>
> +enum addr_type_t {
> + UNICAST_ADDR,
> + MULTICAST_ADDR,
> + ANYCAST_ADDR,
> +};
> +
> struct inet6_fill_args {
> u32 portid;
> u32 seq;
> int event;
> unsigned int flags;
> int netnsid;
> + enum addr_type_t type;
> };
>
> static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
> @@ -4930,39 +4937,28 @@ static int inet6_fill_ifacaddr(struct sk_buff *skb, struct ifacaddr6 *ifaca,
> return 0;
> }
>
> -enum addr_type_t {
> - UNICAST_ADDR,
> - MULTICAST_ADDR,
> - ANYCAST_ADDR,
> -};
> -
> /* called with rcu_read_lock() */
> static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
> - struct netlink_callback *cb, enum addr_type_t type,
> - int s_ip_idx, int *p_ip_idx, int netnsid)
> + struct netlink_callback *cb,
> + int s_ip_idx, int *p_ip_idx,
> + struct inet6_fill_args *fillargs)
> {
> - struct inet6_fill_args fillargs = {
> - .portid = NETLINK_CB(cb->skb).portid,
> - .seq = cb->nlh->nlmsg_seq,
> - .flags = NLM_F_MULTI,
> - .netnsid = netnsid,
> - };
> struct ifmcaddr6 *ifmca;
> struct ifacaddr6 *ifaca;
> int err = 1;
> int ip_idx = *p_ip_idx;
>
> read_lock_bh(&idev->lock);
> - switch (type) {
> + switch (fillargs->type) {
> case UNICAST_ADDR: {
> struct inet6_ifaddr *ifa;
> - fillargs.event = RTM_NEWADDR;
> + fillargs->event = RTM_NEWADDR;
>
> /* unicast address incl. temp addr */
> list_for_each_entry(ifa, &idev->addr_list, if_list) {
> if (++ip_idx < s_ip_idx)
> continue;
> - err = inet6_fill_ifaddr(skb, ifa, &fillargs);
> + err = inet6_fill_ifaddr(skb, ifa, fillargs);
> if (err < 0)
> break;
> nl_dump_check_consistent(cb, nlmsg_hdr(skb));
> @@ -4970,26 +4966,26 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
> break;
> }
> case MULTICAST_ADDR:
> - fillargs.event = RTM_GETMULTICAST;
> + fillargs->event = RTM_GETMULTICAST;
>
> /* multicast address */
> for (ifmca = idev->mc_list; ifmca;
> ifmca = ifmca->next, ip_idx++) {
> if (ip_idx < s_ip_idx)
> continue;
> - err = inet6_fill_ifmcaddr(skb, ifmca, &fillargs);
> + err = inet6_fill_ifmcaddr(skb, ifmca, fillargs);
> if (err < 0)
> break;
> }
> break;
> case ANYCAST_ADDR:
> - fillargs.event = RTM_GETANYCAST;
> + fillargs->event = RTM_GETANYCAST;
> /* anycast address */
> for (ifaca = idev->ac_list; ifaca;
> ifaca = ifaca->aca_next, ip_idx++) {
> if (ip_idx < s_ip_idx)
> continue;
> - err = inet6_fill_ifacaddr(skb, ifaca, &fillargs);
> + err = inet6_fill_ifacaddr(skb, ifaca, fillargs);
> if (err < 0)
> break;
> }
> @@ -5005,10 +5001,16 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
> static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> enum addr_type_t type)
> {
> + struct inet6_fill_args fillargs = {
> + .portid = NETLINK_CB(cb->skb).portid,
> + .seq = cb->nlh->nlmsg_seq,
> + .flags = NLM_F_MULTI,
> + .netnsid = -1,
> + .type = type,
> + };
> struct net *net = sock_net(skb->sk);
> struct nlattr *tb[IFA_MAX+1];
> struct net *tgt_net = net;
> - int netnsid = -1;
> int h, s_h;
> int idx, ip_idx;
> int s_idx, s_ip_idx;
> @@ -5023,11 +5025,14 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> ifa_ipv6_policy, NULL) >= 0) {
> if (tb[IFA_TARGET_NETNSID]) {
> - netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> + fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
>
> - tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid);
> + tgt_net = rtnl_get_net_ns_capable(skb->sk,
> + fillargs.netnsid);
> if (IS_ERR(tgt_net))
> return PTR_ERR(tgt_net);
> +
> + fillargs.flags |= NLM_F_DUMP_FILTERED;
> }
> }
>
> @@ -5046,8 +5051,8 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> if (!idev)
> goto cont;
>
> - if (in6_dump_addrs(idev, skb, cb, type,
> - s_ip_idx, &ip_idx, netnsid) < 0)
> + if (in6_dump_addrs(idev, skb, cb, s_ip_idx, &ip_idx,
> + &fillargs) < 0)
> goto done;
> cont:
> idx++;
> @@ -5058,7 +5063,7 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> cb->args[0] = h;
> cb->args[1] = idx;
> cb->args[2] = ip_idx;
> - if (netnsid >= 0)
> + if (fillargs.netnsid >= 0)
> put_net(tgt_net);
>
> return skb->len;
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net/netlink: Pass extack to dump callbacks
From: Christian Brauner @ 2018-09-28 18:43 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20180928154502.9983-2-dsahern@kernel.org>
On Fri, Sep 28, 2018 at 08:44:58AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Pass extack to dump callbacks by adding extack to netlink_dump_control
> and transferring to netlink_callback. Update rtnetlink as the first
> user.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
I like the idea of passing down extack.
Acked-by: Christian Brauner <christian@brauner.io>
> ---
> include/linux/netlink.h | 2 ++
> net/core/rtnetlink.c | 1 +
> net/netlink/af_netlink.c | 1 +
> 3 files changed, 4 insertions(+)
>
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 71f121b66ca8..8fc90308a653 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -176,6 +176,7 @@ struct netlink_callback {
> void *data;
> /* the module that dump function belong to */
> struct module *module;
> + struct netlink_ext_ack *extack;
> u16 family;
> u16 min_dump_alloc;
> unsigned int prev_seq, seq;
> @@ -197,6 +198,7 @@ struct netlink_dump_control {
> int (*done)(struct netlink_callback *);
> void *data;
> struct module *module;
> + struct netlink_ext_ack *extack;
> u16 min_dump_alloc;
> };
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 35162e1b06ad..da91b38297d3 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4689,6 +4689,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
> .dump = dumpit,
> .min_dump_alloc = min_dump_alloc,
> .module = owner,
> + .extack = extack
> };
> err = netlink_dump_start(rtnl, skb, nlh, &c);
> /* netlink_dump_start() will keep a reference on
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index e3a0538ec0be..7d9e735b32c4 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2307,6 +2307,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
> cb->module = control->module;
> cb->min_dump_alloc = control->min_dump_alloc;
> cb->skb = skb;
> + cb->extack = control->extack;
>
> if (control->start) {
> ret = control->start(cb);
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH RFC net-next 4/5] net/ipv4: Update inet_dump_ifaddr to support NLM_F_DUMP_PROPER_HDR
From: David Ahern @ 2018-09-28 18:43 UTC (permalink / raw)
To: Christian Brauner, dsahern; +Cc: netdev, davem, jbenc, stephen
In-Reply-To: <20180928184115.n7xotgooia6xjfkh@brauner.io>
On 9/28/18 12:41 PM, Christian Brauner wrote:
>> @@ -1683,15 +1683,45 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>> s_idx = idx = cb->args[1];
>> s_ip_idx = ip_idx = cb->args[2];
>>
>> - if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
>> - ifa_ipv4_policy, NULL) >= 0) {
>> - if (tb[IFA_TARGET_NETNSID]) {
>> - fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
>> + if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
>> + struct nlattr *tb[IFA_MAX+1];
>> + struct ifaddrmsg *ifm;
>> + int err, i;
>> +
>> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
>> + NL_SET_ERR_MSG(extack, "Invalid header");
>> + return -EINVAL;
>> + }
>> +
>> + ifm = (struct ifaddrmsg *) nlmsg_data(cb->nlh);
>> + if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
>> + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
>> + return -EINVAL;
>> + }
>> + if (ifm->ifa_index) {
>> + NL_SET_ERR_MSG(extack, "Filter by device index not supported");
>> + return -EINVAL;
>> + }
>> + err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
>> + ifa_ipv4_policy, NULL);
>> + if (err < 0)
>> + return err;
>>
>> - tgt_net = rtnl_get_net_ns_capable(skb->sk,
>> - fillargs.netnsid);
>> - if (IS_ERR(tgt_net))
>> - return PTR_ERR(tgt_net);
>> + for (i = 0; i < IFA_MAX; ++i) {
>> + if (i == IFA_TARGET_NETNSID) {
>> + fillargs.netnsid = nla_get_s32(tb[i]);
>> +
>> + tgt_net = rtnl_get_net_ns_capable(skb->sk,
>> + fillargs.netnsid);
>> + if (IS_ERR(tgt_net))
>> + return PTR_ERR(tgt_net);
>> +
>> + fillargs.flags |= NLM_F_DUMP_FILTERED;
>> + }
>> + if (tb[i]) {
>> + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
>> + return -EINVAL;
>> + }
>
> That loop doesn't do what it promises, no? Shouldn't it be:
your right, that should be:
} else if (tb[i]) {
^ permalink raw reply
* Re: [PATCH RFC net-next 5/5] net/ipv6: Update inet6_dump_addr to support NLM_F_DUMP_PROPER_HDR
From: Christian Brauner @ 2018-09-28 18:42 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20180928154502.9983-6-dsahern@kernel.org>
On Fri, Sep 28, 2018 at 08:45:02AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Update inet6_dump_addr to check for NLM_F_DUMP_PROPER_HDR in the netlink
> message header. If the flag is set, the dump request is expected to have
> an ifaddrmsg struct as the header potentially followed by one or more
> attributes. Any data passed in the header or as an attribute is taken as
> a request to influence the data returned. Only values suppored by the
> dump handler are allowed to be non-0 or set in the request. At the moment
> only the IFA_TARGET_NETNSID attribute is supported. Follow on patches
> will support for other fields (e.g., honor ifa_index and only return data
> for the given device index).
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> net/ipv6/addrconf.c | 50 ++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 375ea9d9869b..888e5a4b8dd2 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -5001,6 +5001,8 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
> static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> enum addr_type_t type)
> {
> + struct netlink_ext_ack *extack = cb->extack;
> + const struct nlmsghdr *nlh = cb->nlh;
> struct inet6_fill_args fillargs = {
> .portid = NETLINK_CB(cb->skb).portid,
> .seq = cb->nlh->nlmsg_seq,
> @@ -5009,7 +5011,6 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> .type = type,
> };
> struct net *net = sock_net(skb->sk);
> - struct nlattr *tb[IFA_MAX+1];
> struct net *tgt_net = net;
> int h, s_h;
> int idx, ip_idx;
> @@ -5022,17 +5023,46 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> s_idx = idx = cb->args[1];
> s_ip_idx = ip_idx = cb->args[2];
>
> - if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> - ifa_ipv6_policy, NULL) >= 0) {
> - if (tb[IFA_TARGET_NETNSID]) {
> - fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> + if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
> + struct nlattr *tb[IFA_MAX+1];
> + struct ifaddrmsg *ifm;
> + int err, i;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
>
> - tgt_net = rtnl_get_net_ns_capable(skb->sk,
> - fillargs.netnsid);
> - if (IS_ERR(tgt_net))
> - return PTR_ERR(tgt_net);
> + ifm = (struct ifaddrmsg *)nlmsg_data(cb->nlh);
> + if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
> + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> + return -EINVAL;
> + }
> + if (ifm->ifa_index) {
> + NL_SET_ERR_MSG(extack, "Filter by device index not supported");
> + return -EINVAL;
> + }
>
> - fillargs.flags |= NLM_F_DUMP_FILTERED;
> + err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> + ifa_ipv6_policy, NULL);
> + if (err < 0)
> + return err;
> +
> + for (i = 0; i < IFA_MAX; ++i) {
> + if (i == IFA_TARGET_NETNSID) {
> + fillargs.netnsid = nla_get_s32(tb[i]);
> +
> + tgt_net = rtnl_get_net_ns_capable(skb->sk,
> + fillargs.netnsid);
> + if (IS_ERR(tgt_net))
> + return PTR_ERR(tgt_net);
> +
> + fillargs.flags |= NLM_F_DUMP_FILTERED;
> + }
> + if (tb[i]) {
> + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> + return -EINVAL;
> + }
Same comment/question as for ipv4. Shouldn't it be:
for (i = 0; i < IFA_MAX; ++i) {
if (i == IFA_TARGET_NETNSID && tb[i]) {
fillargs.netnsid = nla_get_s32(tb[i]);
tgt_net = rtnl_get_net_ns_capable(skb->sk,
fillargs.netnsid);
if (IS_ERR(tgt_net))
return PTR_ERR(tgt_net);
fillargs.flags |= NLM_F_DUMP_FILTERED;
continue;
}
if (tb[i]) {
NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
return -EINVAL;
}
> }
> }
>
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH RFC net-next 4/5] net/ipv4: Update inet_dump_ifaddr to support NLM_F_DUMP_PROPER_HDR
From: Christian Brauner @ 2018-09-28 18:41 UTC (permalink / raw)
To: dsahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20180928154502.9983-5-dsahern@kernel.org>
On Fri, Sep 28, 2018 at 08:45:01AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Update inet_dump_ifaddr to check for NLM_F_DUMP_PROPER_HDR in the netlink
> message header. If the flag is set, the dump request is expected to have
> an ifaddrmsg struct as the header potentially followed by one or more
> attributes. Any data passed in the header or as an attribute is taken as
> a request to influence the data returned. Only values suppored by the
> dump handler are allowed to be non-0 or set in the request. At the moment
> only the IFA_TARGET_NETNSID attribute is supported. Follow on patches
> will support for other fields (e.g., honor ifa_index and only return data
> for the given device index).
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> net/ipv4/devinet.c | 52 +++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 44d931a3cd50..1e06a21cd8f4 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1661,15 +1661,15 @@ static int inet_fill_ifaddr(struct sk_buff *skb, struct in_ifaddr *ifa,
>
> static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + struct netlink_ext_ack *extack = cb->extack;
> + const struct nlmsghdr *nlh = cb->nlh;
> struct inet_fill_args fillargs = {
> .portid = NETLINK_CB(cb->skb).portid,
> - .seq = cb->nlh->nlmsg_seq,
> + .seq = nlh->nlmsg_seq,
> .event = RTM_NEWADDR,
> - .flags = NLM_F_MULTI,
> .netnsid = -1,
> };
> struct net *net = sock_net(skb->sk);
> - struct nlattr *tb[IFA_MAX+1];
> struct net *tgt_net = net;
> int h, s_h;
> int idx, s_idx;
> @@ -1683,15 +1683,45 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> s_idx = idx = cb->args[1];
> s_ip_idx = ip_idx = cb->args[2];
>
> - if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> - ifa_ipv4_policy, NULL) >= 0) {
> - if (tb[IFA_TARGET_NETNSID]) {
> - fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> + if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
> + struct nlattr *tb[IFA_MAX+1];
> + struct ifaddrmsg *ifm;
> + int err, i;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + ifm = (struct ifaddrmsg *) nlmsg_data(cb->nlh);
> + if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
> + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> + return -EINVAL;
> + }
> + if (ifm->ifa_index) {
> + NL_SET_ERR_MSG(extack, "Filter by device index not supported");
> + return -EINVAL;
> + }
> + err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> + ifa_ipv4_policy, NULL);
> + if (err < 0)
> + return err;
>
> - tgt_net = rtnl_get_net_ns_capable(skb->sk,
> - fillargs.netnsid);
> - if (IS_ERR(tgt_net))
> - return PTR_ERR(tgt_net);
> + for (i = 0; i < IFA_MAX; ++i) {
> + if (i == IFA_TARGET_NETNSID) {
> + fillargs.netnsid = nla_get_s32(tb[i]);
> +
> + tgt_net = rtnl_get_net_ns_capable(skb->sk,
> + fillargs.netnsid);
> + if (IS_ERR(tgt_net))
> + return PTR_ERR(tgt_net);
> +
> + fillargs.flags |= NLM_F_DUMP_FILTERED;
> + }
> + if (tb[i]) {
> + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> + return -EINVAL;
> + }
That loop doesn't do what it promises, no? Shouldn't it be:
for (i = 0; i < IFA_MAX; ++i) {
if (i == IFA_TARGET_NETNSID && tb[IFA_TARGET_NETNSID]) {
fillargs.netnsid = nla_get_s32(tb[i]);
tgt_net = rtnl_get_net_ns_capable(skb->sk,
fillargs.netnsid);
if (IS_ERR(tgt_net))
return PTR_ERR(tgt_net);
fillargs.flags |= NLM_F_DUMP_FILTERED;
continue;
}
if (tb[i]) {
NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
return -EINVAL;
}
}
> }
>
> --
> 2.11.0
>
^ permalink raw reply
* Re: bond: take rcu lock in bond_poll_controller
From: Eric Dumazet @ 2018-09-28 18:38 UTC (permalink / raw)
To: Dave Jones, netdev
In-Reply-To: <20180928182410.slizlkx6fiwkyb76@codemonkey.org.uk>
On 09/28/2018 11:24 AM, Dave Jones wrote:
> Callers of bond_for_each_slave_rcu are expected to hold the rcu lock,
> otherwise a trace like below is shown
>
> WARNING: CPU: 2 PID: 179 at net/core/dev.c:6567 netdev_lower_get_next_private_rcu+0x34/0x40
> CPU: 2 PID: 179 Comm: kworker/u16:15 Not tainted 4.19.0-rc5-backup+ #1
>
>
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Dave Jones <davej@codemonkey.org.uk>
>
You forgot to change patch title.
^ permalink raw reply
* Re: [Patch net-next v3] net_sched: change tcf_del_walker() to take idrinfo->lock
From: Cong Wang @ 2018-09-28 18:29 UTC (permalink / raw)
To: Ido Schimmel
Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim,
Vlad Buslov
In-Reply-To: <20180928181114.GA28797@splinter>
On Fri, Sep 28, 2018 at 11:11 AM Ido Schimmel <idosch@idosch.org> wrote:
> I don't think this will work given the reference count already dropped
> to 0, which is why the template deletion function was invoked. I didn't
> test the patch, but I don't see what would prevent the chain from being
> freed.
Good catch! So I should just "move" tcf_chain_destroy() into cls_flower.
A patch is being compiled now.
^ permalink raw reply
* bond: take rcu lock in bond_poll_controller
From: Dave Jones @ 2018-09-28 18:24 UTC (permalink / raw)
To: netdev
In-Reply-To: <20180928161631.3pab3m2k6zvoemnz@codemonkey.org.uk>
Callers of bond_for_each_slave_rcu are expected to hold the rcu lock,
otherwise a trace like below is shown
WARNING: CPU: 2 PID: 179 at net/core/dev.c:6567 netdev_lower_get_next_private_rcu+0x34/0x40
CPU: 2 PID: 179 Comm: kworker/u16:15 Not tainted 4.19.0-rc5-backup+ #1
Workqueue: bond0 bond_mii_monitor
RIP: 0010:netdev_lower_get_next_private_rcu+0x34/0x40
Code: 48 89 fb e8 fe 29 63 ff 85 c0 74 1e 48 8b 45 00 48 81 c3 c0 00 00 00 48 8b 00 48 39 d8 74 0f 48 89 45 00 48 8b 40 f8 5b 5d c3 <0f> 0b eb de 31 c0 eb f5 0f 1f 40 00 0f 1f 44 00 00 48 8>
RSP: 0018:ffffc9000087fa68 EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff880429614560 RCX: 0000000000000000
RDX: 0000000000000001 RSI: 00000000ffffffff RDI: ffffffffa184ada0
RBP: ffffc9000087fa80 R08: 0000000000000001 R09: 0000000000000000
R10: ffffc9000087f9f0 R11: ffff880429798040 R12: ffff8804289d5980
R13: ffffffffa1511f60 R14: 00000000000000c8 R15: 00000000ffffffff
FS: 0000000000000000(0000) GS:ffff88042f880000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f4b78fce180 CR3: 000000018180f006 CR4: 00000000001606e0
Call Trace:
bond_poll_controller+0x52/0x170
netpoll_poll_dev+0x79/0x290
netpoll_send_skb_on_dev+0x158/0x2c0
netpoll_send_udp+0x2d5/0x430
write_ext_msg+0x1e0/0x210
console_unlock+0x3c4/0x630
vprintk_emit+0xfa/0x2f0
printk+0x52/0x6e
? __netdev_printk+0x12b/0x220
netdev_info+0x64/0x80
? bond_3ad_set_carrier+0xe9/0x180
bond_select_active_slave+0x1fc/0x310
bond_mii_monitor+0x709/0x9b0
process_one_work+0x221/0x5e0
worker_thread+0x4f/0x3b0
kthread+0x100/0x140
? process_one_work+0x5e0/0x5e0
? kthread_delayed_work_timer_fn+0x90/0x90
ret_from_fork+0x24/0x30
Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Dave Jones <davej@codemonkey.org.uk>
--
v3: Do this in netpoll_send_skb_on_dev as Cong suggests.
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 3219a2932463..4f9494381635 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -330,6 +330,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
/* It is up to the caller to keep npinfo alive. */
struct netpoll_info *npinfo;
+ rcu_read_lock();
lockdep_assert_irqs_disabled();
npinfo = rcu_dereference_bh(np->dev->npinfo);
@@ -374,6 +375,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
skb_queue_tail(&npinfo->txq, skb);
schedule_delayed_work(&npinfo->tx_work,0);
}
+ rcu_read_unlock();
}
EXPORT_SYMBOL(netpoll_send_skb_on_dev);
^ permalink raw reply related
* [net-next 4/5] tipc: add SYN bit to connection setup messages
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
To: davem, netdev
Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
canh.d.luu, ying.xue, tipc-discussion
In-Reply-To: <1538159002-11800-1-git-send-email-jon.maloy@ericsson.com>
Messages intended for intitating a connection are currently
indistinguishable from regular datagram messages. The TIPC
protocol specification defines bit 17 in word 0 as a SYN bit
to allow sanity check of such messages in the listening socket,
but this has so far never been implemented.
We do that in this commit.
Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/msg.h | 10 ++++++++++
net/tipc/node.h | 12 +++++++-----
net/tipc/socket.c | 5 +++++
3 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index a4e944d..cd64d7b 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -216,6 +216,16 @@ static inline void msg_set_non_seq(struct tipc_msg *m, u32 n)
msg_set_bits(m, 0, 20, 1, n);
}
+static inline int msg_is_syn(struct tipc_msg *m)
+{
+ return msg_bits(m, 0, 17, 1);
+}
+
+static inline void msg_set_syn(struct tipc_msg *m, u32 d)
+{
+ msg_set_bits(m, 0, 17, 1, d);
+}
+
static inline int msg_dest_droppable(struct tipc_msg *m)
{
return msg_bits(m, 0, 19, 1);
diff --git a/net/tipc/node.h b/net/tipc/node.h
index 48b3298..03f5efb 100644
--- a/net/tipc/node.h
+++ b/net/tipc/node.h
@@ -45,6 +45,7 @@
/* Optional capabilities supported by this code version
*/
enum {
+ TIPC_SYN_BIT = (1),
TIPC_BCAST_SYNCH = (1 << 1),
TIPC_BCAST_STATE_NACK = (1 << 2),
TIPC_BLOCK_FLOWCTL = (1 << 3),
@@ -53,11 +54,12 @@ enum {
TIPC_LINK_PROTO_SEQNO = (1 << 6)
};
-#define TIPC_NODE_CAPABILITIES (TIPC_BCAST_SYNCH | \
- TIPC_BCAST_STATE_NACK | \
- TIPC_BCAST_RCAST | \
- TIPC_BLOCK_FLOWCTL | \
- TIPC_NODE_ID128 | \
+#define TIPC_NODE_CAPABILITIES (TIPC_SYN_BIT | \
+ TIPC_BCAST_SYNCH | \
+ TIPC_BCAST_STATE_NACK | \
+ TIPC_BCAST_RCAST | \
+ TIPC_BLOCK_FLOWCTL | \
+ TIPC_NODE_ID128 | \
TIPC_LINK_PROTO_SEQNO)
#define INVALID_BEARER_ID -1
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 094cfdc..89d6dc0 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1319,6 +1319,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
tsk->conn_type = dest->addr.name.name.type;
tsk->conn_instance = dest->addr.name.name.instance;
}
+ msg_set_syn(hdr, 1);
}
seq = &dest->addr.nameseq;
@@ -1478,6 +1479,7 @@ static void tipc_sk_finish_conn(struct tipc_sock *tsk, u32 peer_port,
struct net *net = sock_net(sk);
struct tipc_msg *msg = &tsk->phdr;
+ msg_set_syn(msg, 0);
msg_set_destnode(msg, peer_node);
msg_set_destport(msg, peer_port);
msg_set_type(msg, TIPC_CONN_MSG);
@@ -2006,6 +2008,9 @@ static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct sk_buff *skb)
return false;
case TIPC_LISTEN:
/* Accept only SYN message */
+ if (!msg_is_syn(hdr) &&
+ tipc_node_get_capabilities(net, onode) & TIPC_SYN_BIT)
+ return false;
if (!con_msg && !err)
return true;
return false;
--
2.1.4
^ permalink raw reply related
* [net-next 2/5] tipc: refactor function tipc_sk_timeout()
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
To: davem, netdev
Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
canh.d.luu, ying.xue, tipc-discussion
In-Reply-To: <1538159002-11800-1-git-send-email-jon.maloy@ericsson.com>
We refactor this function as a preparation for the coming commits in
the same series.
Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/socket.c | 62 ++++++++++++++++++++++++++++++++++---------------------
1 file changed, 38 insertions(+), 24 deletions(-)
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 3f03ddd..8152a81 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2545,43 +2545,57 @@ static int tipc_shutdown(struct socket *sock, int how)
return res;
}
+static void tipc_sk_check_probing_state(struct sock *sk,
+ struct sk_buff_head *list)
+{
+ struct tipc_sock *tsk = tipc_sk(sk);
+ u32 pnode = tsk_peer_node(tsk);
+ u32 pport = tsk_peer_port(tsk);
+ u32 self = tsk_own_node(tsk);
+ u32 oport = tsk->portid;
+ struct sk_buff *skb;
+
+ if (tsk->probe_unacked) {
+ tipc_set_sk_state(sk, TIPC_DISCONNECTING);
+ sk->sk_err = ECONNABORTED;
+ tipc_node_remove_conn(sock_net(sk), pnode, pport);
+ sk->sk_state_change(sk);
+ return;
+ }
+ /* Prepare new probe */
+ skb = tipc_msg_create(CONN_MANAGER, CONN_PROBE, INT_H_SIZE, 0,
+ pnode, self, pport, oport, TIPC_OK);
+ if (skb)
+ __skb_queue_tail(list, skb);
+ tsk->probe_unacked = true;
+ sk_reset_timer(sk, &sk->sk_timer, jiffies + CONN_PROBING_INTV);
+}
+
static void tipc_sk_timeout(struct timer_list *t)
{
struct sock *sk = from_timer(sk, t, sk_timer);
struct tipc_sock *tsk = tipc_sk(sk);
- u32 peer_port = tsk_peer_port(tsk);
- u32 peer_node = tsk_peer_node(tsk);
- u32 own_node = tsk_own_node(tsk);
- u32 own_port = tsk->portid;
- struct net *net = sock_net(sk);
- struct sk_buff *skb = NULL;
+ u32 pnode = tsk_peer_node(tsk);
+ struct sk_buff_head list;
+ skb_queue_head_init(&list);
bh_lock_sock(sk);
- if (!tipc_sk_connected(sk))
- goto exit;
/* Try again later if socket is busy */
if (sock_owned_by_user(sk)) {
sk_reset_timer(sk, &sk->sk_timer, jiffies + HZ / 20);
- goto exit;
+ bh_unlock_sock(sk);
+ return;
}
- if (tsk->probe_unacked) {
- tipc_set_sk_state(sk, TIPC_DISCONNECTING);
- tipc_node_remove_conn(net, peer_node, peer_port);
- sk->sk_state_change(sk);
- goto exit;
- }
- /* Send new probe */
- skb = tipc_msg_create(CONN_MANAGER, CONN_PROBE, INT_H_SIZE, 0,
- peer_node, own_node, peer_port, own_port,
- TIPC_OK);
- tsk->probe_unacked = true;
- sk_reset_timer(sk, &sk->sk_timer, jiffies + CONN_PROBING_INTV);
-exit:
+ if (sk->sk_state == TIPC_ESTABLISHED)
+ tipc_sk_check_probing_state(sk, &list);
+
bh_unlock_sock(sk);
- if (skb)
- tipc_node_xmit_skb(net, skb, peer_node, own_port);
+
+ if (!skb_queue_empty(&list))
+ tipc_node_xmit(sock_net(sk), &list, pnode, tsk->portid);
+
sock_put(sk);
}
--
2.1.4
^ permalink raw reply related
* [net-next 3/5] tipc: refactor function tipc_sk_filter_connect()
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
To: davem, netdev
Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
canh.d.luu, ying.xue, tipc-discussion
In-Reply-To: <1538159002-11800-1-git-send-email-jon.maloy@ericsson.com>
We refactor the function tipc_sk_filter_connect(), both to make it
more readable and as a preparation for the next commit.
Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/socket.c | 101 +++++++++++++++++++++++-------------------------------
1 file changed, 43 insertions(+), 58 deletions(-)
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 8152a81..094cfdc 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1959,91 +1959,76 @@ static void tipc_sk_proto_rcv(struct sock *sk,
}
/**
- * tipc_filter_connect - Handle incoming message for a connection-based socket
+ * tipc_sk_filter_connect - check incoming message for a connection-based socket
* @tsk: TIPC socket
- * @skb: pointer to message buffer. Set to NULL if buffer is consumed
- *
- * Returns true if everything ok, false otherwise
+ * @skb: pointer to message buffer.
+ * Returns true if message should be added to receive queue, false otherwise
*/
static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct sk_buff *skb)
{
struct sock *sk = &tsk->sk;
struct net *net = sock_net(sk);
struct tipc_msg *hdr = buf_msg(skb);
- u32 pport = msg_origport(hdr);
- u32 pnode = msg_orignode(hdr);
+ bool con_msg = msg_connected(hdr);
+ u32 pport = tsk_peer_port(tsk);
+ u32 pnode = tsk_peer_node(tsk);
+ u32 oport = msg_origport(hdr);
+ u32 onode = msg_orignode(hdr);
+ int err = msg_errcode(hdr);
if (unlikely(msg_mcast(hdr)))
return false;
switch (sk->sk_state) {
case TIPC_CONNECTING:
- /* Accept only ACK or NACK message */
- if (unlikely(!msg_connected(hdr))) {
- if (pport != tsk_peer_port(tsk) ||
- pnode != tsk_peer_node(tsk))
- return false;
-
- tipc_set_sk_state(sk, TIPC_DISCONNECTING);
- sk->sk_err = ECONNREFUSED;
- sk->sk_state_change(sk);
- return true;
- }
-
- if (unlikely(msg_errcode(hdr))) {
- tipc_set_sk_state(sk, TIPC_DISCONNECTING);
- sk->sk_err = ECONNREFUSED;
- sk->sk_state_change(sk);
- return true;
- }
-
- if (unlikely(!msg_isdata(hdr))) {
- tipc_set_sk_state(sk, TIPC_DISCONNECTING);
- sk->sk_err = EINVAL;
- sk->sk_state_change(sk);
- return true;
+ /* Setup ACK */
+ if (likely(con_msg)) {
+ if (err)
+ break;
+ tipc_sk_finish_conn(tsk, oport, onode);
+ msg_set_importance(&tsk->phdr, msg_importance(hdr));
+ /* ACK+ message with data is added to receive queue */
+ if (msg_data_sz(hdr))
+ return true;
+ /* Empty ACK-, - wake up sleeping connect() and drop */
+ sk->sk_data_ready(sk);
+ msg_set_dest_droppable(hdr, 1);
+ return false;
}
+ /* Ignore connectionless message if not from listening socket */
+ if (oport != pport || onode != pnode)
+ return false;
- tipc_sk_finish_conn(tsk, msg_origport(hdr), msg_orignode(hdr));
- msg_set_importance(&tsk->phdr, msg_importance(hdr));
-
- /* If 'ACK+' message, add to socket receive queue */
- if (msg_data_sz(hdr))
- return true;
-
- /* If empty 'ACK-' message, wake up sleeping connect() */
- sk->sk_data_ready(sk);
-
- /* 'ACK-' message is neither accepted nor rejected: */
- msg_set_dest_droppable(hdr, 1);
- return false;
-
+ /* Rejected SYN - abort */
+ break;
case TIPC_OPEN:
case TIPC_DISCONNECTING:
- break;
+ return false;
case TIPC_LISTEN:
/* Accept only SYN message */
- if (!msg_connected(hdr) && !(msg_errcode(hdr)))
+ if (!con_msg && !err)
return true;
- break;
+ return false;
case TIPC_ESTABLISHED:
/* Accept only connection-based messages sent by peer */
- if (unlikely(!tsk_peer_msg(tsk, hdr)))
+ if (likely(con_msg && !err && pport == oport && pnode == onode))
+ return true;
+ if (!tsk_peer_msg(tsk, hdr))
return false;
-
- if (unlikely(msg_errcode(hdr))) {
- tipc_set_sk_state(sk, TIPC_DISCONNECTING);
- /* Let timer expire on it's own */
- tipc_node_remove_conn(net, tsk_peer_node(tsk),
- tsk->portid);
- sk->sk_state_change(sk);
- }
+ if (!err)
+ return true;
+ tipc_set_sk_state(sk, TIPC_DISCONNECTING);
+ tipc_node_remove_conn(net, pnode, tsk->portid);
+ sk->sk_state_change(sk);
return true;
default:
pr_err("Unknown sk_state %u\n", sk->sk_state);
}
-
- return false;
+ /* Abort connection setup attempt */
+ tipc_set_sk_state(sk, TIPC_DISCONNECTING);
+ sk->sk_err = ECONNREFUSED;
+ sk->sk_state_change(sk);
+ return true;
}
/**
--
2.1.4
^ permalink raw reply related
* [net-next 0/5] tipc: make connection setup more robust
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
To: davem, netdev
Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
canh.d.luu, ying.xue, tipc-discussion
In this series we make a few improvements to the connection setup and
probing mechanism, culminating in the last commit where we make it
possible for a client socket to make multiple setup attempts in case
it encounters receive buffer overflow at the listener socket.
Jon Maloy (4):
tipc: refactor function tipc_msg_reverse()
tipc: refactor function tipc_sk_timeout()
tipc: refactor function tipc_sk_filter_connect()
tipc: add SYN bit to connection setup messages
Tung Nguyen (1):
tipc: buffer overflow handling in listener socket
net/tipc/msg.c | 78 ++++++++++++--------
net/tipc/msg.h | 11 +++
net/tipc/node.h | 12 ++--
net/tipc/socket.c | 207 ++++++++++++++++++++++++++++++++----------------------
4 files changed, 190 insertions(+), 118 deletions(-)
--
2.1.4
^ permalink raw reply
* [net-next 1/5] tipc: refactor function tipc_msg_reverse()
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
To: davem, netdev
Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
canh.d.luu, ying.xue, tipc-discussion
In-Reply-To: <1538159002-11800-1-git-send-email-jon.maloy@ericsson.com>
The function tipc_msg_reverse() is reversing the header of a message
while reusing the original buffer. We have seen at several occasions
that this may have unfortunate side effects when the buffer to be
reversed is a clone.
In one of the following commits we will again need to reverse cloned
buffers, so this is the right time to permanently eliminate this
problem. In this commit we let the said function always consume the
original buffer and replace it with a new one when applicable.
Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/msg.c | 58 ++++++++++++++++++++++++++++------------------------------
1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index b618910..00fbb5c 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -499,54 +499,52 @@ bool tipc_msg_make_bundle(struct sk_buff **skb, struct tipc_msg *msg,
/**
* tipc_msg_reverse(): swap source and destination addresses and add error code
* @own_node: originating node id for reversed message
- * @skb: buffer containing message to be reversed; may be replaced.
+ * @skb: buffer containing message to be reversed; will be consumed
* @err: error code to be set in message, if any
- * Consumes buffer at failure
+ * Replaces consumed buffer with new one when successful
* Returns true if success, otherwise false
*/
bool tipc_msg_reverse(u32 own_node, struct sk_buff **skb, int err)
{
struct sk_buff *_skb = *skb;
- struct tipc_msg *hdr;
- struct tipc_msg ohdr;
- int dlen;
+ struct tipc_msg *_hdr, *hdr;
+ int hlen, dlen;
if (skb_linearize(_skb))
goto exit;
- hdr = buf_msg(_skb);
- dlen = min_t(uint, msg_data_sz(hdr), MAX_FORWARD_SIZE);
- if (msg_dest_droppable(hdr))
+ _hdr = buf_msg(_skb);
+ dlen = min_t(uint, msg_data_sz(_hdr), MAX_FORWARD_SIZE);
+ hlen = msg_hdr_sz(_hdr);
+
+ if (msg_dest_droppable(_hdr))
goto exit;
- if (msg_errcode(hdr))
+ if (msg_errcode(_hdr))
goto exit;
- /* Take a copy of original header before altering message */
- memcpy(&ohdr, hdr, msg_hdr_sz(hdr));
-
- /* Never return SHORT header; expand by replacing buffer if necessary */
- if (msg_short(hdr)) {
- *skb = tipc_buf_acquire(BASIC_H_SIZE + dlen, GFP_ATOMIC);
- if (!*skb)
- goto exit;
- memcpy((*skb)->data + BASIC_H_SIZE, msg_data(hdr), dlen);
- kfree_skb(_skb);
- _skb = *skb;
- hdr = buf_msg(_skb);
- memcpy(hdr, &ohdr, BASIC_H_SIZE);
- msg_set_hdr_sz(hdr, BASIC_H_SIZE);
- }
+ /* Never return SHORT header */
+ if (hlen == SHORT_H_SIZE)
+ hlen = BASIC_H_SIZE;
- /* Now reverse the concerned fields */
+ /* Allocate new buffer to return */
+ *skb = tipc_buf_acquire(hlen + dlen, GFP_ATOMIC);
+ if (!*skb)
+ goto exit;
+ memcpy((*skb)->data, _skb->data, msg_hdr_sz(_hdr));
+ memcpy((*skb)->data + hlen, msg_data(_hdr), dlen);
+
+ /* Build reverse header in new buffer */
+ hdr = buf_msg(*skb);
+ msg_set_hdr_sz(hdr, hlen);
msg_set_errcode(hdr, err);
msg_set_non_seq(hdr, 0);
- msg_set_origport(hdr, msg_destport(&ohdr));
- msg_set_destport(hdr, msg_origport(&ohdr));
- msg_set_destnode(hdr, msg_prevnode(&ohdr));
+ msg_set_origport(hdr, msg_destport(_hdr));
+ msg_set_destport(hdr, msg_origport(_hdr));
+ msg_set_destnode(hdr, msg_prevnode(_hdr));
msg_set_prevnode(hdr, own_node);
msg_set_orignode(hdr, own_node);
- msg_set_size(hdr, msg_hdr_sz(hdr) + dlen);
- skb_trim(_skb, msg_size(hdr));
+ msg_set_size(hdr, hlen + dlen);
skb_orphan(_skb);
+ kfree_skb(_skb);
return true;
exit:
kfree_skb(_skb);
--
2.1.4
^ permalink raw reply related
* [net-next 5/5] tipc: buffer overflow handling in listener socket
From: Jon Maloy @ 2018-09-28 18:23 UTC (permalink / raw)
To: davem, netdev
Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
canh.d.luu, ying.xue, tipc-discussion
In-Reply-To: <1538159002-11800-1-git-send-email-jon.maloy@ericsson.com>
From: Tung Nguyen <tung.q.nguyen@dektech.com.au>
Default socket receive buffer size for a listener socket is 2Mb. For
each arriving empty SYN, the linux kernel allocates a 768 bytes buffer.
This means that a listener socket can serve maximum 2700 simultaneous
empty connection setup requests before it hits a receive buffer
overflow, and much fewer if the SYN is carrying any significant
amount of data.
When this happens the setup request is rejected, and the client
receives an ECONNREFUSED error.
This commit mitigates this problem by letting the client socket try to
retransmit the SYN message multiple times when it sees it rejected with
the code TIPC_ERR_OVERLOAD. Retransmission is done at random intervals
in the range of [100 ms, setup_timeout / 4], as many times as there is
room for within the setup timeout limit.
Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au>
Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/msg.c | 20 ++++++++++++++++++++
net/tipc/msg.h | 1 +
net/tipc/socket.c | 49 +++++++++++++++++++++++++++++++++++++++++++------
3 files changed, 64 insertions(+), 6 deletions(-)
diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 00fbb5c..f48e585 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -525,6 +525,10 @@ bool tipc_msg_reverse(u32 own_node, struct sk_buff **skb, int err)
if (hlen == SHORT_H_SIZE)
hlen = BASIC_H_SIZE;
+ /* Don't return data along with SYN+, - sender has a clone */
+ if (msg_is_syn(_hdr) && err == TIPC_ERR_OVERLOAD)
+ dlen = 0;
+
/* Allocate new buffer to return */
*skb = tipc_buf_acquire(hlen + dlen, GFP_ATOMIC);
if (!*skb)
@@ -552,6 +556,22 @@ bool tipc_msg_reverse(u32 own_node, struct sk_buff **skb, int err)
return false;
}
+bool tipc_msg_skb_clone(struct sk_buff_head *msg, struct sk_buff_head *cpy)
+{
+ struct sk_buff *skb, *_skb;
+
+ skb_queue_walk(msg, skb) {
+ _skb = skb_clone(skb, GFP_ATOMIC);
+ if (!_skb) {
+ __skb_queue_purge(cpy);
+ pr_err_ratelimited("Failed to clone buffer chain\n");
+ return false;
+ }
+ __skb_queue_tail(cpy, _skb);
+ }
+ return true;
+}
+
/**
* tipc_msg_lookup_dest(): try to find new destination for named message
* @skb: the buffer containing the message.
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index cd64d7b..a2879e6 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -980,6 +980,7 @@ bool tipc_msg_pskb_copy(u32 dst, struct sk_buff_head *msg,
struct sk_buff_head *cpy);
void __tipc_skb_queue_sorted(struct sk_buff_head *list, u16 seqno,
struct sk_buff *skb);
+bool tipc_msg_skb_clone(struct sk_buff_head *msg, struct sk_buff_head *cpy);
static inline u16 buf_seqno(struct sk_buff *skb)
{
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 89d6dc0..595c500 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -47,7 +47,7 @@
#include "netlink.h"
#include "group.h"
-#define CONN_TIMEOUT_DEFAULT 8000 /* default connect timeout = 8s */
+#define CONN_TIMEOUT_DEFAULT 8000 /* default connect timeout = 8s */
#define CONN_PROBING_INTV msecs_to_jiffies(3600000) /* [ms] => 1 h */
#define TIPC_FWD_MSG 1
#define TIPC_MAX_PORT 0xffffffff
@@ -80,7 +80,6 @@ struct sockaddr_pair {
* @publications: list of publications for port
* @blocking_link: address of the congested link we are currently sleeping on
* @pub_count: total # of publications port has made during its lifetime
- * @probing_state:
* @conn_timeout: the time we can wait for an unresponded setup request
* @dupl_rcvcnt: number of bytes counted twice, in both backlog and rcv queue
* @cong_link_cnt: number of congested links
@@ -102,8 +101,8 @@ struct tipc_sock {
struct list_head cong_links;
struct list_head publications;
u32 pub_count;
- uint conn_timeout;
atomic_t dupl_rcvcnt;
+ u16 conn_timeout;
bool probe_unacked;
u16 cong_link_cnt;
u16 snt_unacked;
@@ -507,6 +506,9 @@ static void __tipc_shutdown(struct socket *sock, int error)
tipc_wait_for_cond(sock, &timeout, (!tsk->cong_link_cnt &&
!tsk_conn_cong(tsk)));
+ /* Remove any pending SYN message */
+ __skb_queue_purge(&sk->sk_write_queue);
+
/* Reject all unreceived messages, except on an active connection
* (which disconnects locally & sends a 'FIN+' to peer).
*/
@@ -1362,6 +1364,8 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
rc = tipc_msg_build(hdr, m, 0, dlen, mtu, &pkts);
if (unlikely(rc != dlen))
return rc;
+ if (unlikely(syn && !tipc_msg_skb_clone(&pkts, &sk->sk_write_queue)))
+ return -ENOMEM;
rc = tipc_node_xmit(net, &pkts, dnode, tsk->portid);
if (unlikely(rc == -ELINKCONG)) {
@@ -1491,6 +1495,7 @@ static void tipc_sk_finish_conn(struct tipc_sock *tsk, u32 peer_port,
tipc_node_add_conn(net, peer_node, tsk->portid, peer_port);
tsk->max_pkt = tipc_node_get_mtu(net, peer_node, tsk->portid);
tsk->peer_caps = tipc_node_get_capabilities(net, peer_node);
+ __skb_queue_purge(&sk->sk_write_queue);
if (tsk->peer_caps & TIPC_BLOCK_FLOWCTL)
return;
@@ -1977,6 +1982,7 @@ static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct sk_buff *skb)
u32 oport = msg_origport(hdr);
u32 onode = msg_orignode(hdr);
int err = msg_errcode(hdr);
+ unsigned long delay;
if (unlikely(msg_mcast(hdr)))
return false;
@@ -2001,8 +2007,18 @@ static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct sk_buff *skb)
if (oport != pport || onode != pnode)
return false;
- /* Rejected SYN - abort */
- break;
+ /* Rejected SYN */
+ if (err != TIPC_ERR_OVERLOAD)
+ break;
+
+ /* Prepare for new setup attempt if we have a SYN clone */
+ if (skb_queue_empty(&sk->sk_write_queue))
+ break;
+ get_random_bytes(&delay, 2);
+ delay %= (tsk->conn_timeout / 4);
+ delay = msecs_to_jiffies(delay + 100);
+ sk_reset_timer(sk, &sk->sk_timer, jiffies + delay);
+ return false;
case TIPC_OPEN:
case TIPC_DISCONNECTING:
return false;
@@ -2561,12 +2577,26 @@ static void tipc_sk_check_probing_state(struct sock *sk,
sk_reset_timer(sk, &sk->sk_timer, jiffies + CONN_PROBING_INTV);
}
+static void tipc_sk_retry_connect(struct sock *sk, struct sk_buff_head *list)
+{
+ struct tipc_sock *tsk = tipc_sk(sk);
+
+ /* Try again later if dest link is congested */
+ if (tsk->cong_link_cnt) {
+ sk_reset_timer(sk, &sk->sk_timer, msecs_to_jiffies(100));
+ return;
+ }
+ /* Prepare SYN for retransmit */
+ tipc_msg_skb_clone(&sk->sk_write_queue, list);
+}
+
static void tipc_sk_timeout(struct timer_list *t)
{
struct sock *sk = from_timer(sk, t, sk_timer);
struct tipc_sock *tsk = tipc_sk(sk);
u32 pnode = tsk_peer_node(tsk);
struct sk_buff_head list;
+ int rc = 0;
skb_queue_head_init(&list);
bh_lock_sock(sk);
@@ -2580,12 +2610,19 @@ static void tipc_sk_timeout(struct timer_list *t)
if (sk->sk_state == TIPC_ESTABLISHED)
tipc_sk_check_probing_state(sk, &list);
+ else if (sk->sk_state == TIPC_CONNECTING)
+ tipc_sk_retry_connect(sk, &list);
bh_unlock_sock(sk);
if (!skb_queue_empty(&list))
- tipc_node_xmit(sock_net(sk), &list, pnode, tsk->portid);
+ rc = tipc_node_xmit(sock_net(sk), &list, pnode, tsk->portid);
+ /* SYN messages may cause link congestion */
+ if (rc == -ELINKCONG) {
+ tipc_dest_push(&tsk->cong_links, pnode, 0);
+ tsk->cong_link_cnt = 1;
+ }
sock_put(sk);
}
--
2.1.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox