* [PATCH] sfp: sfp_read: split-up request when hw rx buffer is too small.
From: René van Dorst @ 2019-01-23 21:20 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
Cc: René van Dorst
Without this patch sfp code retries to read the full struct sfp_eeprom_id
id out of the SFP eeprom. Sizeof(id) is 96 bytes.
My i2c hardware, Mediatek mt7621, has a rx buffer of 64 bytes.
So sfp_read gets -NOSUPPORTED back on his turn return -EAGAIN.
Same issue is with the SFP_EXT_STATUS data which is 92 bytes.
By split-up the request in multiple smaller requests with a max size of i2c
max_read_len, we can readout the SFP module successfully.
Tested with MT7621 and two Fiberstore modules SFP-GB-GE-T and SFP-GE-BX.
Signed-off-by: René van Dorst <opensource@vdorst.com>
---
drivers/net/phy/sfp.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index fd8bb998ae52..1352a19571cd 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -367,7 +367,28 @@ static void sfp_set_state(struct sfp *sfp, unsigned int state)
static int sfp_read(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
{
- return sfp->read(sfp, a2, addr, buf, len);
+ const struct i2c_adapter_quirks *q = sfp->i2c->quirks;
+ int ret;
+ size_t rx_bytes = 0;
+
+ /* Many i2c hw have limited rx buffers, split-up request when needed. */
+ while ((q->max_read_len) && (len > q->max_read_len)) {
+ ret = sfp->read(sfp, a2, addr, buf, q->max_read_len);
+ if (ret < 0)
+ return ret;
+ rx_bytes += ret;
+ addr += q->max_read_len;
+ buf += q->max_read_len;
+ len -= q->max_read_len;
+ }
+
+ ret = sfp->read(sfp, a2, addr, buf, len);
+ if (ret < 0)
+ return ret;
+
+ rx_bytes += ret;
+
+ return rx_bytes;
}
static int sfp_write(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] sfp: sfp_read: split-up request when hw rx buffer is too small.
From: Andrew Lunn @ 2019-01-23 21:32 UTC (permalink / raw)
To: René van Dorst
Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
In-Reply-To: <20190123212046.13020-1-opensource@vdorst.com>
On Wed, Jan 23, 2019 at 10:20:46PM +0100, René van Dorst wrote:
> Without this patch sfp code retries to read the full struct sfp_eeprom_id
> id out of the SFP eeprom. Sizeof(id) is 96 bytes.
> My i2c hardware, Mediatek mt7621, has a rx buffer of 64 bytes.
> So sfp_read gets -NOSUPPORTED back on his turn return -EAGAIN.
> Same issue is with the SFP_EXT_STATUS data which is 92 bytes.
>
> By split-up the request in multiple smaller requests with a max size of i2c
> max_read_len, we can readout the SFP module successfully.
>
> Tested with MT7621 and two Fiberstore modules SFP-GB-GE-T and SFP-GE-BX.
>
> Signed-off-by: René van Dorst <opensource@vdorst.com>
> ---
> drivers/net/phy/sfp.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index fd8bb998ae52..1352a19571cd 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -367,7 +367,28 @@ static void sfp_set_state(struct sfp *sfp, unsigned int state)
>
> static int sfp_read(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
> {
> - return sfp->read(sfp, a2, addr, buf, len);
> + const struct i2c_adapter_quirks *q = sfp->i2c->quirks;
> + int ret;
> + size_t rx_bytes = 0;
> +
> + /* Many i2c hw have limited rx buffers, split-up request when needed. */
> + while ((q->max_read_len) && (len > q->max_read_len)) {
> + ret = sfp->read(sfp, a2, addr, buf, q->max_read_len);
Hi René
I think you want to pass MIN(len, q->max_read_len) to read().
> + if (ret < 0)
> + return ret;
> + rx_bytes += ret;
> + addr += q->max_read_len;
> + buf += q->max_read_len;
> + len -= q->max_read_len;
I would prefer you add ret, not q->max_read_len. There is a danger it
returned less bytes than you asked for.
> + }
> +
> + ret = sfp->read(sfp, a2, addr, buf, len);
> + if (ret < 0)
> + return ret;
> +
> + rx_bytes += ret;
> +
> + return rx_bytes;
> }
>
> static int sfp_write(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
Doesn't write need the same handling?
Thanks
Andrew
^ permalink raw reply
* Re: UBSAN: Undefined behaviour in net/ipv4/ip_output.c
From: Willem de Bruijn @ 2019-01-23 21:33 UTC (permalink / raw)
To: Kyungtae Kim
Cc: David Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Byoungyoung Lee, DaeRyong Jeong, syzkaller, Network Development,
LKML
In-Reply-To: <CAEAjamsix7hdT7d3qvZ1mVEFJ+OV-pxCX4Xzc4gWFA4_7YKRQw@mail.gmail.com>
On Mon, Jan 21, 2019 at 2:25 PM Kyungtae Kim <kt0755@gmail.com> wrote:
>
> I'm reporting a bug in linux-5.0-rc2: "UBSAN: Undefined behaviour in
> net/ipv4/ip_output.c"
>
> kernel config: https://kt0755.github.io/etc/config-5.0-rc2
> repro: https://kt0755.github.io/etc/repro.b6a11.c
Thanks for the report.
That reproducer does not seem to exercise system call sendmsg. Did you
mean to share another? Perhaps one that users SO_SNDBUFFORCE or
sysctl_wmem_max.
When constructing a reproducer, I did easily trigger an UBSAN warning
when setting SO_SNDBUFFORCE to INT_MAX - 1, so I can imagine that this
one can trigger, as well. As long as sk_sndbuf can be set to any
integer value, we'll have to be careful with any such multiplication.
> Integer overflow happened in __ip_append_data() when 2 * sk->sk_sndbuf
> (at line 1004)
> is larger than the boundary of the destination (i.e., int).
> Some sanity check code right before it would help.
>
> =========================================
> UBSAN: Undefined behaviour in net/ipv4/ip_output.c:1004:11
> signed integer overflow:
> 1282607372 * 2 cannot be represented in type 'int'
> CPU: 0 PID: 8465 Comm: syz-executor4 Not tainted 5.0.0-rc2 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xb1/0x118 lib/dump_stack.c:113
> ubsan_epilogue+0x12/0x94 lib/ubsan.c:159
> handle_overflow+0x1cf/0x21a lib/ubsan.c:190
> __ubsan_handle_mul_overflow+0x2a/0x35 lib/ubsan.c:214
> __ip_append_data+0x30ed/0x3350 net/ipv4/ip_output.c:1004
> ip_append_data.part.18+0xf3/0x170 net/ipv4/ip_output.c:1220
> ip_append_data+0x63/0x80 net/ipv4/ip_output.c:1209
> raw_sendmsg+0xd99/0x2b60 net/ipv4/raw.c:670
> inet_sendmsg+0xfc/0x620 net/ipv4/af_inet.c:798
> sock_sendmsg_nosec net/socket.c:621 [inline]
> sock_sendmsg+0xdd/0x130 net/socket.c:631
> ___sys_sendmsg+0x7b3/0x950 net/socket.c:2116
> __sys_sendmsg+0xfc/0x1d0 net/socket.c:2154
> __do_sys_sendmsg net/socket.c:2163 [inline]
> __se_sys_sendmsg net/socket.c:2161 [inline]
> __x64_sys_sendmsg+0x83/0xc0 net/socket.c:2161
> do_syscall_64+0xbe/0x4f0 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x4497b9
> Code: e8 8c 9f 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48
> 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d
> 01 f0 ff ff 0f 83 9b 6b fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007f0d0100bc68 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 00007f0d0100c6cc RCX: 00000000004497b9
> RDX: 0000000000000000 RSI: 0000000020000500 RDI: 0000000000000013
> RBP: 000000000071bea0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
> R13: 00000000000089a0 R14: 00000000006f1a40 R15: 00007f0d0100c700
> =========================================
>
> Thanks,
> Kyungtae
^ permalink raw reply
* Re: Question: pause mode disabled for marvell 88e151x phy
From: Russell King - ARM Linux admin @ 2019-01-23 22:41 UTC (permalink / raw)
To: Yunsheng Lin
Cc: Andrew Lunn, Florian Fainelli, davem@davemloft.net,
netdev@vger.kernel.org, Weiwei Deng, Yisen.Zhuang@huawei.com,
huangdaode@hisilicon.com, lipeng321@huawei.com,
salil.mehta@huawei.com, lijianhua 00216010,
linux-kernel@vger.kernel.org
In-Reply-To: <6e8034d9-cd60-2846-ef33-f8a04069e1ee@huawei.com>
On Sat, Jan 05, 2019 at 11:28:19AM +0800, Yunsheng Lin wrote:
> On 2018/12/17 22:36, Russell King - ARM Linux wrote:
> > I'll try to do further diagnosis over Christmas in case I've missed
> > something, but I suspect it may be one of those "weird behaviour" issues
> > where the safest action is to disable pause mode as per my commit -
> > which is far saner than having mismatched pause status on either end
> > of a link. However, given that Marvell specs are all NDA-only, it's
> > very difficult to investigate beyond "this is the observed behaviour".
>
> Hi,
>
> Is there any update on the further diagnosis?
Hi,
I've finally been able to do some further diagnosis (with a 'scope).
It would appear that the network adapter had PHY polling enabled,
which meant that it overwrote the PHYs advertisement register
during negotiation. I thought I'd checked that scenario, but alas
clearing the PHY poll enable bit on its own doesn't stop it polling!
I'll send a revert for the commit shortly.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH] sfp: sfp_read: split-up request when hw rx buffer is too small.
From: Florian Fainelli @ 2019-01-23 22:43 UTC (permalink / raw)
To: René van Dorst, Andrew Lunn, Heiner Kallweit,
David S. Miller, netdev
In-Reply-To: <20190123212046.13020-1-opensource@vdorst.com>
On 1/23/19 1:20 PM, René van Dorst wrote:
> Without this patch sfp code retries to read the full struct sfp_eeprom_id
> id out of the SFP eeprom. Sizeof(id) is 96 bytes.
> My i2c hardware, Mediatek mt7621, has a rx buffer of 64 bytes.
> So sfp_read gets -NOSUPPORTED back on his turn return -EAGAIN.
> Same issue is with the SFP_EXT_STATUS data which is 92 bytes.
>
> By split-up the request in multiple smaller requests with a max size of i2c
> max_read_len, we can readout the SFP module successfully.
>
> Tested with MT7621 and two Fiberstore modules SFP-GB-GE-T and SFP-GE-BX.
>
> Signed-off-by: René van Dorst <opensource@vdorst.com>
> ---
> drivers/net/phy/sfp.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index fd8bb998ae52..1352a19571cd 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -367,7 +367,28 @@ static void sfp_set_state(struct sfp *sfp, unsigned int state)
>
> static int sfp_read(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
> {
> - return sfp->read(sfp, a2, addr, buf, len);
> + const struct i2c_adapter_quirks *q = sfp->i2c->quirks;
> + int ret;
> + size_t rx_bytes = 0;
> +
> + /* Many i2c hw have limited rx buffers, split-up request when needed. */
> + while ((q->max_read_len) && (len > q->max_read_len)) {
> + ret = sfp->read(sfp, a2, addr, buf, q->max_read_len);
> + if (ret < 0)
> + return ret;
> + rx_bytes += ret;
> + addr += q->max_read_len;
> + buf += q->max_read_len;
> + len -= q->max_read_len;
> + }
sfp->read defaults to sfp_i2c_read() but it is possible to override that
by registering a custom sfp_bus instance (nothing does it today, but
that could obviously change), so there is no guarantee that
sfp->i2c->quirks is applicable unless sfp_i2c_read() is used.
sfp_i2c_read() is presumably where the max_read_len splitting should
occur, or better yet, should not i2c_transfer() take care of that on its
own? That way there would be no layering violation of having to fetch
the quirk bitmask for the i2c adapter being used, that is something that
should belong in the core i2c framework.
> +
> + ret = sfp->read(sfp, a2, addr, buf, len);
> + if (ret < 0)
> + return ret;
> +
> + rx_bytes += ret;
> +
> + return rx_bytes;
> }
>
> static int sfp_write(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
>
--
Florian
^ permalink raw reply
* Re: [PATCH] net: altera_tse: fix msgdma_tx_completion on non-zero fill_level case
From: Dalon L Westergreen @ 2019-01-23 22:47 UTC (permalink / raw)
To: thor.thayer, Atsushi Nemoto, netdev; +Cc: Tomonori Sakita
In-Reply-To: <d624692a-83d8-a74a-b9bd-d9cff79f362d@linux.intel.com>
On Tue, 2019-01-22 at 11:20 -0600, Thor Thayer wrote:
> On 1/21/19 2:29 AM, Atsushi Nemoto wrote:
> > From: Tomonori Sakita <tomonori.sakita@sord.co.jp>
> >
> > If fill_level was not zero and status was not BUSY,
> > result of "tx_prod - tx_cons - inuse" might be zero.
> > Subtracting 1 unconditionally results invalid negative return value
> > on this case.
> > The subtraction is not needed if fill_level was not zero.
> >
> > Signed-off-by: Tomonori Sakita <tomonori.sakita@sord.co.jp>
> > Signed-off-by: Atsushi Nemoto <atsushi.nemoto@sord.co.jp>
> > ---
> > drivers/net/ethernet/altera/altera_msgdma.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/altera/altera_msgdma.c
> > b/drivers/net/ethernet/altera/altera_msgdma.c
> > index 0fb986b..3df73d3 100644
> > --- a/drivers/net/ethernet/altera/altera_msgdma.c
> > +++ b/drivers/net/ethernet/altera/altera_msgdma.c
> > @@ -145,7 +145,7 @@ u32 msgdma_tx_completions(struct altera_tse_private
> > *priv)
> > & 0xffff;
> >
> > if (inuse) { /* Tx FIFO is not empty */
> > - ready = priv->tx_prod - priv->tx_cons - inuse - 1;
> > + ready = priv->tx_prod - priv->tx_cons - inuse;
dont think my last email went through..
I am not sure about this. This register indicates the number of entries
still to be processed by the dma. the -1 is intended to represent the
decriptor currently being processed. If ready is
priv->tx_prod - priv->tx_cons - inuse couldn't you end up processing 1
too many packets? IE: ready is 1 greater then the actual completed
packets?
I do agree that we should not be returning a negative value, but i dont
think i agree removing the -1 is the answer. perhaps just check that ready
is greater than 0?
--dalon
> > } else {
> > /* Check for buffered last packet */
> > status = csrrd32(priv->tx_dma_csr, msgdma_csroffs(status));
> >
>
> I'm adding Dalon who has done a lot of work on msgdma. I'd like to get
> his comments.
>
> Thanks,
>
> Thor
>
^ permalink raw reply
* Re: [PATCH net] ax25: fix possible use-after-free
From: Cong Wang @ 2019-01-23 23:25 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, Ralf Baechle, syzbot
In-Reply-To: <20190122184059.220252-1-edumazet@google.com>
On Tue, Jan 22, 2019 at 10:41 AM 'Eric Dumazet' via syzkaller
<syzkaller@googlegroups.com> wrote:
>
> syzbot found that ax25 routes where not properly protected
> against concurrent use [1].
>
> In this particular report the bug happened while
> copying ax25->digipeat.
>
> Fix this problem by making sure we call ax25_get_route()
> while ax25_route_lock is held, so that no modification
> could happen while using the route.
ax25_route_lock_use() is a read lock, so two ax25_rt_autobind()
could still enter the same critical section?
>
> The current two ax25_get_route() callers do not sleep,
> so this change should be fine.
>
> Once we do that, ax25_get_route() no longer needs to
> grab a reference on the found route.
.
After your patch, ax25_hold_route() has no callers while
ax25_put_route() still does. Is ->refcount always 1?
Thanks.
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] selftests: bpf: break up test_verifier
From: Alexei Starovoitov @ 2019-01-23 23:32 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: daniel, netdev, oss-drivers
In-Reply-To: <20190121184339.29485-3-jakub.kicinski@netronome.com>
On Mon, Jan 21, 2019 at 10:43:38AM -0800, Jakub Kicinski wrote:
> Break up the first 10 kLoC of test verifier test cases
> out into smaller files. Looks like git line counting
> gets a little flismy above 16 bit integers, so we need
> two commits to break up test_verifier.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Acked-by: Jiong Wang <jiong.wang@netronome.com>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 10100 ----------------
> tools/testing/selftests/bpf/verifier/and.c | 53 +
> .../selftests/bpf/verifier/array_access.c | 238 +
...
> - {
> - "DIV64 by 0, zero check",
> - .insns = {
> - BPF_MOV32_IMM(BPF_REG_0, 42),
> - BPF_MOV32_IMM(BPF_REG_1, 0),
> - BPF_MOV32_IMM(BPF_REG_2, 1),
> - BPF_ALU64_REG(BPF_DIV, BPF_REG_2, BPF_REG_1),
> - BPF_EXIT_INSN(),
> - },
> - .result = ACCEPT,
> - .retval = 42,
> - },
...
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/and.c
> @@ -0,0 +1,53 @@
> +{
> + "invalid and of negative number",
> + .insns = {
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_LD_MAP_FD(BPF_REG_1, 0),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> + BPF_FUNC_map_lookup_elem),
> + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
> + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> + BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> + BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> + BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> + BPF_ST_MEM(BPF_DW, BPF_REG_0, 0,
> + offsetof(struct test_val, foo)),
> + BPF_EXIT_INSN(),
> + },
> + .fixup_map_hash_48b = { 3 },
> + .errstr = "R0 max value is outside of the array range",
> + .result = REJECT,
> + .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> +},
I like the removal of one tab, but since we're refactoring the whole thing
can we remove another tab from instructions?
In many cases we wrap the lines which makes tests a bit harder to read/write
since jmp offsets don't easily add from current line number.
In addition I would propose to represet LD_MAP_FD as two lines too,
but that's optional. I'm thinking to try that for new tests.
So how about the following indent:
{
"invalid and of negative number",
.insns = {
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
BPF_LD_MAP_FD(BPF_REG_1,
0),
BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
BPF_EXIT_INSN(),
},
.fixup_map_hash_48b = { 3 },
.errstr = "R0 max value is outside of the array range",
.result = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},
Notice how calls and offsetof() fits into 80 char.
Another alternative is to switch to two spaces instead of tab:
{
"invalid and of negative number",
.insns = {
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
BPF_LD_MAP_FD(BPF_REG_1,
0),
BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
BPF_EXIT_INSN(),
},
.fixup_map_hash_48b = { 3 },
.errstr = "R0 max value is outside of the array range",
.result = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},
Thoughts?
^ permalink raw reply
* Re: [PATCH net] ax25: fix possible use-after-free
From: Eric Dumazet @ 2019-01-23 23:42 UTC (permalink / raw)
To: Cong Wang, Eric Dumazet; +Cc: David S . Miller, netdev, Ralf Baechle, syzbot
In-Reply-To: <CAM_iQpXoPGFj=ey7_aYKhhfPdL1SMxQj=adydxVYqwPJhBOSpg@mail.gmail.com>
On 01/23/2019 03:25 PM, Cong Wang wrote:
> On Tue, Jan 22, 2019 at 10:41 AM 'Eric Dumazet' via syzkaller
> <syzkaller@googlegroups.com> wrote:
>>
>> syzbot found that ax25 routes where not properly protected
>> against concurrent use [1].
>>
>> In this particular report the bug happened while
>> copying ax25->digipeat.
>>
>> Fix this problem by making sure we call ax25_get_route()
>> while ax25_route_lock is held, so that no modification
>> could happen while using the route.
>
> ax25_route_lock_use() is a read lock, so two ax25_rt_autobind()
> could still enter the same critical section?
>
Not sure I understand your concern.
The two ax25_rt_autobind() would only read the route contents,
so that should be fine ?
>
>>
>> The current two ax25_get_route() callers do not sleep,
>> so this change should be fine.
>>
>> Once we do that, ax25_get_route() no longer needs to
>> grab a reference on the found route.
> .
>
> After your patch, ax25_hold_route() has no callers while
> ax25_put_route() still does. Is ->refcount always 1?
Yes, the plan is to remove this refcount in net-next.
^ permalink raw reply
* Re: [PATCH bpf-next v5 4/9] tools: bpftool: add probes for eBPF program types
From: Andrey Ignatov @ 2019-01-23 23:44 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, netdev@vger.kernel.org,
oss-drivers@netronome.com, Arnaldo Carvalho de Melo,
Jesper Dangaard Brouer, Stanislav Fomichev
In-Reply-To: <20190117152758.14883-5-quentin.monnet@netronome.com>
Quentin Monnet <quentin.monnet@netronome.com> [Thu, 2019-01-17 07:28 -0800]:
> Introduce probes for supported BPF program types in libbpf, and call it
> from bpftool to test what types are available on the system. The probe
> simply consists in loading a very basic program of that type and see if
> the verifier complains or not.
>
> Sample output:
>
> # bpftool feature probe kernel
> ...
> Scanning eBPF program types...
> eBPF program_type socket_filter is available
> eBPF program_type kprobe is available
> eBPF program_type sched_cls is available
> ...
>
> # bpftool --json --pretty feature probe kernel
> {
> ...
> "program_types": {
> "have_socket_filter_prog_type": true,
> "have_kprobe_prog_type": true,
> "have_sched_cls_prog_type": true,
> ...
> }
> }
Hi Quentin,
Awesome work! I wondef if you have plan to add similar functionality for
attach types?
For example BPF_CGROUP_INET{4,6}_CONNECT were added in 4.17 but
BPF_CGROUP_UDP{4,6}_SENDMSG later in 4.18 even though they correspond to
same prog type (BPF_PROG_TYPE_CGROUP_SOCK_ADDR).
> v5:
> - In libbpf.map, move global symbol to a new LIBBPF_0.0.2 section.
> - Rename (non-API function) prog_load() as probe_load().
>
> v3:
> - Get kernel version for checking kprobes availability from libbpf
> instead of from bpftool. Do not pass kernel_version as an argument
> when calling libbpf probes.
> - Use a switch with all enum values for setting specific program
> parameters just before probing, so that gcc complains at compile time
> (-Wswitch-enum) if new prog types were added to the kernel but libbpf
> was not updated.
> - Add a comment in libbpf.h about setrlimit() usage to allow many
> consecutive probe attempts.
>
> v2:
> - Move probes from bpftool to libbpf.
> - Remove C-style macros output from this patch.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Stanislav Fomichev <sdf@google.com>
> ---
> tools/bpf/bpftool/feature.c | 48 +++++++++++++++++-
> tools/lib/bpf/Build | 2 +-
> tools/lib/bpf/libbpf.h | 11 ++++
> tools/lib/bpf/libbpf.map | 5 ++
> tools/lib/bpf/libbpf_probes.c | 95 +++++++++++++++++++++++++++++++++++
> 5 files changed, 159 insertions(+), 2 deletions(-)
> create mode 100644 tools/lib/bpf/libbpf_probes.c
>
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index 4a2867439ab7..d6508dde4808 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> /* Copyright (c) 2019 Netronome Systems, Inc. */
>
> +#include <ctype.h>
> #include <errno.h>
> #include <string.h>
> #include <unistd.h>
> @@ -11,6 +12,7 @@
> #include <linux/limits.h>
>
> #include <bpf.h>
> +#include <libbpf.h>
>
> #include "main.h"
>
> @@ -83,6 +85,17 @@ print_start_section(const char *json_title, const char *plain_title)
> }
> }
>
> +static void
> +print_end_then_start_section(const char *json_title, const char *plain_title)
> +{
> + if (json_output)
> + jsonw_end_object(json_wtr);
> + else
> + printf("\n");
> +
> + print_start_section(json_title, plain_title);
> +}
> +
> /* Probing functions */
>
> static int read_procfs(const char *path)
> @@ -403,9 +416,33 @@ static bool probe_bpf_syscall(void)
> return res;
> }
>
> +static void probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types)
> +{
> + const char *plain_comment = "eBPF program_type ";
> + char feat_name[128], plain_desc[128];
> + size_t maxlen;
> + bool res;
> +
> + res = bpf_probe_prog_type(prog_type, 0);
> +
> + supported_types[prog_type] |= res;
> +
> + maxlen = sizeof(plain_desc) - strlen(plain_comment) - 1;
> + if (strlen(prog_type_name[prog_type]) > maxlen) {
> + p_info("program type name too long");
> + return;
> + }
> +
> + sprintf(feat_name, "have_%s_prog_type", prog_type_name[prog_type]);
> + sprintf(plain_desc, "%s%s", plain_comment, prog_type_name[prog_type]);
> + print_bool_feature(feat_name, plain_desc, res);
> +}
> +
> static int do_probe(int argc, char **argv)
> {
> enum probe_component target = COMPONENT_UNSPEC;
> + bool supported_types[128] = {};
> + unsigned int i;
>
> /* Detection assumes user has sufficient privileges (CAP_SYS_ADMIN).
> * Let's approximate, and restrict usage to root user only.
> @@ -460,8 +497,17 @@ static int do_probe(int argc, char **argv)
> print_start_section("syscall_config",
> "Scanning system call availability...");
>
> - probe_bpf_syscall();
> + if (!probe_bpf_syscall())
> + /* bpf() syscall unavailable, don't probe other BPF features */
> + goto exit_close_json;
> +
> + print_end_then_start_section("program_types",
> + "Scanning eBPF program types...");
> +
> + for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
> + probe_prog_type(i, supported_types);
>
> +exit_close_json:
> if (json_output) {
> /* End current "section" of probes */
> jsonw_end_object(json_wtr);
> diff --git a/tools/lib/bpf/Build b/tools/lib/bpf/Build
> index 197b40f5b5c6..bfd9bfc82c3b 100644
> --- a/tools/lib/bpf/Build
> +++ b/tools/lib/bpf/Build
> @@ -1 +1 @@
> -libbpf-y := libbpf.o bpf.o nlattr.o btf.o libbpf_errno.o str_error.o netlink.o bpf_prog_linfo.o
> +libbpf-y := libbpf.o bpf.o nlattr.o btf.o libbpf_errno.o str_error.o netlink.o bpf_prog_linfo.o libbpf_probes.o
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 5f68d7b75215..8e63821109ab 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -355,6 +355,17 @@ LIBBPF_API const struct bpf_line_info *
> bpf_prog_linfo__lfind(const struct bpf_prog_linfo *prog_linfo,
> __u32 insn_off, __u32 nr_skip);
>
> +/*
> + * Probe for supported system features
> + *
> + * Note that running many of these probes in a short amount of time can cause
> + * the kernel to reach the maximal size of lockable memory allowed for the
> + * user, causing subsequent probes to fail. In this case, the caller may want
> + * to adjust that limit with setrlimit().
> + */
> +LIBBPF_API bool bpf_probe_prog_type(enum bpf_prog_type prog_type,
> + __u32 ifindex);
> +
> #ifdef __cplusplus
> } /* extern "C" */
> #endif
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index cd02cd4e2cc3..c7ec3ffa24e9 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -124,3 +124,8 @@ LIBBPF_0.0.1 {
> local:
> *;
> };
> +
> +LIBBPF_0.0.2 {
> + global:
> + bpf_probe_prog_type;
> +} LIBBPF_0.0.1;
> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> new file mode 100644
> index 000000000000..056c0c186f2a
> --- /dev/null
> +++ b/tools/lib/bpf/libbpf_probes.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> +/* Copyright (c) 2019 Netronome Systems, Inc. */
> +
> +#include <errno.h>
> +#include <unistd.h>
> +#include <sys/utsname.h>
> +
> +#include <linux/filter.h>
> +#include <linux/kernel.h>
> +
> +#include "bpf.h"
> +#include "libbpf.h"
> +
> +static int get_kernel_version(void)
> +{
> + int version, subversion, patchlevel;
> + struct utsname utsn;
> +
> + /* Return 0 on failure, and attempt to probe with empty kversion */
> + if (uname(&utsn))
> + return 0;
> +
> + if (sscanf(utsn.release, "%d.%d.%d",
> + &version, &subversion, &patchlevel) != 3)
> + return 0;
> +
> + return (version << 16) + (subversion << 8) + patchlevel;
> +}
> +
> +static void
> +probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
> + size_t insns_cnt, char *buf, size_t buf_len, __u32 ifindex)
> +{
> + struct bpf_load_program_attr xattr = {};
> + int fd;
> +
> + switch (prog_type) {
> + case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
> + xattr.expected_attach_type = BPF_CGROUP_INET4_CONNECT;
> + break;
> + case BPF_PROG_TYPE_KPROBE:
> + xattr.kern_version = get_kernel_version();
> + break;
> + case BPF_PROG_TYPE_UNSPEC:
> + case BPF_PROG_TYPE_SOCKET_FILTER:
> + case BPF_PROG_TYPE_SCHED_CLS:
> + case BPF_PROG_TYPE_SCHED_ACT:
> + case BPF_PROG_TYPE_TRACEPOINT:
> + case BPF_PROG_TYPE_XDP:
> + case BPF_PROG_TYPE_PERF_EVENT:
> + case BPF_PROG_TYPE_CGROUP_SKB:
> + case BPF_PROG_TYPE_CGROUP_SOCK:
> + case BPF_PROG_TYPE_LWT_IN:
> + case BPF_PROG_TYPE_LWT_OUT:
> + case BPF_PROG_TYPE_LWT_XMIT:
> + case BPF_PROG_TYPE_SOCK_OPS:
> + case BPF_PROG_TYPE_SK_SKB:
> + case BPF_PROG_TYPE_CGROUP_DEVICE:
> + case BPF_PROG_TYPE_SK_MSG:
> + case BPF_PROG_TYPE_RAW_TRACEPOINT:
> + case BPF_PROG_TYPE_LWT_SEG6LOCAL:
> + case BPF_PROG_TYPE_LIRC_MODE2:
> + case BPF_PROG_TYPE_SK_REUSEPORT:
> + case BPF_PROG_TYPE_FLOW_DISSECTOR:
> + default:
> + break;
> + }
> +
> + xattr.prog_type = prog_type;
> + xattr.insns = insns;
> + xattr.insns_cnt = insns_cnt;
> + xattr.license = "GPL";
> + xattr.prog_ifindex = ifindex;
> +
> + fd = bpf_load_program_xattr(&xattr, buf, buf_len);
> + if (fd >= 0)
> + close(fd);
> +}
> +
> +bool bpf_probe_prog_type(enum bpf_prog_type prog_type, __u32 ifindex)
> +{
> + struct bpf_insn insns[2] = {
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN()
> + };
> +
> + if (ifindex && prog_type == BPF_PROG_TYPE_SCHED_CLS)
> + /* nfp returns -EINVAL on exit(0) with TC offload */
> + insns[0].imm = 2;
> +
> + errno = 0;
> + probe_load(prog_type, insns, ARRAY_SIZE(insns), NULL, 0, ifindex);
> +
> + return errno != EINVAL && errno != EOPNOTSUPP;
> +}
> --
> 2.17.1
>
--
Andrey Ignatov
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] selftests: bpf: break up test_verifier
From: Jakub Kicinski @ 2019-01-23 23:47 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: daniel, netdev, oss-drivers
In-Reply-To: <20190123233203.jzxw2xkqwygraqui@ast-mbp.dhcp.thefacebook.com>
On Wed, 23 Jan 2019 15:32:05 -0800, Alexei Starovoitov wrote:
> On Mon, Jan 21, 2019 at 10:43:38AM -0800, Jakub Kicinski wrote:
> > Break up the first 10 kLoC of test verifier test cases
> > out into smaller files. Looks like git line counting
> > gets a little flismy above 16 bit integers, so we need
> > two commits to break up test_verifier.
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Acked-by: Jiong Wang <jiong.wang@netronome.com>
> > ---
> > tools/testing/selftests/bpf/test_verifier.c | 10100 ----------------
> > tools/testing/selftests/bpf/verifier/and.c | 53 +
> > .../selftests/bpf/verifier/array_access.c | 238 +
> ...
> > - {
> > - "DIV64 by 0, zero check",
> > - .insns = {
> > - BPF_MOV32_IMM(BPF_REG_0, 42),
> > - BPF_MOV32_IMM(BPF_REG_1, 0),
> > - BPF_MOV32_IMM(BPF_REG_2, 1),
> > - BPF_ALU64_REG(BPF_DIV, BPF_REG_2, BPF_REG_1),
> > - BPF_EXIT_INSN(),
> > - },
> > - .result = ACCEPT,
> > - .retval = 42,
> > - },
> ...
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/verifier/and.c
> > @@ -0,0 +1,53 @@
> > +{
> > + "invalid and of negative number",
> > + .insns = {
> > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > + BPF_LD_MAP_FD(BPF_REG_1, 0),
> > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> > + BPF_FUNC_map_lookup_elem),
> > + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
> > + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> > + BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> > + BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> > + BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> > + BPF_ST_MEM(BPF_DW, BPF_REG_0, 0,
> > + offsetof(struct test_val, foo)),
> > + BPF_EXIT_INSN(),
> > + },
> > + .fixup_map_hash_48b = { 3 },
> > + .errstr = "R0 max value is outside of the array range",
> > + .result = REJECT,
> > + .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> > +},
>
> I like the removal of one tab, but since we're refactoring the whole thing
> can we remove another tab from instructions?
> In many cases we wrap the lines which makes tests a bit harder to read/write
> since jmp offsets don't easily add from current line number.
> In addition I would propose to represet LD_MAP_FD as two lines too,
> but that's optional. I'm thinking to try that for new tests.
> So how about the following indent:
> {
> "invalid and of negative number",
> .insns = {
> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> BPF_LD_MAP_FD(BPF_REG_1,
> 0),
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
> BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
> BPF_EXIT_INSN(),
> },
> .fixup_map_hash_48b = { 3 },
> .errstr = "R0 max value is outside of the array range",
> .result = REJECT,
> .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> },
>
> Notice how calls and offsetof() fits into 80 char.
> Another alternative is to switch to two spaces instead of tab:
> {
> "invalid and of negative number",
> .insns = {
> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> BPF_LD_MAP_FD(BPF_REG_1,
> 0),
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
> BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
> BPF_EXIT_INSN(),
> },
> .fixup_map_hash_48b = { 3 },
> .errstr = "R0 max value is outside of the array range",
> .result = REJECT,
> .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> },
>
> Thoughts?
I'd vote for option (1). The two spaces don't seem to provide good
enough visual separation, IMHO we'd either have to go to 4, or put the
opening bracket on it's own line GNU style.
^ permalink raw reply
* Re: [PATCH iproute2-next v2] devlink: Add health command support
From: David Ahern @ 2019-01-24 0:27 UTC (permalink / raw)
To: Aya Levin, netdev@vger.kernel.org, David S. Miller, Jiri Pirko
Cc: Moshe Shemesh, Eran Ben Elisha, Tal Alon, Ariel Almog
In-Reply-To: <801c73a5-3978-6541-8edc-f7e3036b3a80@mellanox.com>
On 1/23/19 4:27 AM, Aya Levin wrote:
>>> @@ -1298,6 +1322,12 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
>>> return -EINVAL;
>>> }
>>>
>>> + if ((o_required & DL_OPT_HEALTH_REPORTER_NAME) &&
>>> + !(o_found & DL_OPT_HEALTH_REPORTER_NAME)) {
>>> + pr_err("Reporter name expected.\n");
>>> + return -EINVAL;
>>> + }
>>
>> I realize your are following suit with this change, but these checks for
>> 'required and not found' are getting really long. There is a better way
>> to do this.
> Do you mean somthing like:
> #define requiered_not_found(o_found, o_required, flag, msg) \
> ({ \
> if ((o_required & flag) && !(o_found & flag)) { \
> pr_err("%s \n", msg); \
> return -EINVAL; \
> } \
> })
>
That's one way.
I was also thinking a helper that does a single loop with an array of
bits and messages to print if required and not found.
That parse function is currently 355 lines long with a lot of repetition.
^ permalink raw reply
* Re: [PATCH net-next 03/10] net: macsec: move some definitions in a dedicated header
From: David Miller @ 2019-01-24 1:00 UTC (permalink / raw)
To: f.fainelli
Cc: antoine.tenart, sd, andrew, hkallweit1, netdev, linux-kernel,
thomas.petazzoni, alexandre.belloni, quentin.schulz,
allan.nielsen
In-Reply-To: <955d1bdf-037e-ae26-a9da-8da2a51a7cbd@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 23 Jan 2019 12:11:37 -0800
> On 1/23/19 7:56 AM, Antoine Tenart wrote:
>> This patch moves some structure, type and identifier definitions into a
>> MACsec specific header. This patch does not modify how the MACsec code
>> is running and only move things around. This is a preparation for the
>> future MACsec hardware offloading support, which will re-use those
>> definitions outside macsec.c.
>>
>> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
>> ---
>> drivers/net/macsec.c | 164 +--------------------------------------
>> include/net/macsec.h | 178 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 179 insertions(+), 163 deletions(-)
>> create mode 100644 include/net/macsec.h
>>
>> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
>> index 56e354305f76..c3a138dd4386 100644
>> --- a/drivers/net/macsec.c
>> +++ b/drivers/net/macsec.c
>> @@ -17,10 +17,9 @@
>> #include <net/sock.h>
>> #include <net/gro_cells.h>
>>
>> +#include <net/macsec.h>
>> #include <uapi/linux/if_macsec.h>
>
> I would probably go with include/linux/if_macsec.h and have
> uapi/linux/if_macsec.h include that file directly. This would be
> consistent with other types of network interfaces: bridge, vlan etc.
Agreed.
^ permalink raw reply
* Re: [PATCH net] ax25: fix possible use-after-free
From: Cong Wang @ 2019-01-24 1:12 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Eric Dumazet, David S . Miller, netdev, Ralf Baechle, syzbot
In-Reply-To: <0dd7561b-533c-5054-816c-38fa8e9fae5f@gmail.com>
On Wed, Jan 23, 2019 at 3:42 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 01/23/2019 03:25 PM, Cong Wang wrote:
> > On Tue, Jan 22, 2019 at 10:41 AM 'Eric Dumazet' via syzkaller
> > <syzkaller@googlegroups.com> wrote:
> >>
> >> syzbot found that ax25 routes where not properly protected
> >> against concurrent use [1].
> >>
> >> In this particular report the bug happened while
> >> copying ax25->digipeat.
> >>
> >> Fix this problem by making sure we call ax25_get_route()
> >> while ax25_route_lock is held, so that no modification
> >> could happen while using the route.
> >
> > ax25_route_lock_use() is a read lock, so two ax25_rt_autobind()
> > could still enter the same critical section?
> >
>
> Not sure I understand your concern.
>
> The two ax25_rt_autobind() would only read the route contents,
> so that should be fine ?
Not sure if it is read-only and safe for the following code:
if (ax25_rt->digipeat != NULL) {
ax25->digipeat = kmemdup(ax25_rt->digipeat, sizeof(ax25_digi),
GFP_ATOMIC);
if (ax25->digipeat == NULL) {
err = -ENOMEM;
goto put;
}
ax25_adjust_path(addr, ax25->digipeat);
}
Maybe we leak memory here at least, not sure...
>
> >
> >>
> >> The current two ax25_get_route() callers do not sleep,
> >> so this change should be fine.
> >>
> >> Once we do that, ax25_get_route() no longer needs to
> >> grab a reference on the found route.
> > .
> >
> > After your patch, ax25_hold_route() has no callers while
> > ax25_put_route() still does. Is ->refcount always 1?
>
> Yes, the plan is to remove this refcount in net-next.
>
Good to know.
Thanks.
^ permalink raw reply
* Re: [Patch iproute2] tc: add performance counters for basic filter
From: Cong Wang @ 2019-01-24 1:14 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Ahern
In-Reply-To: <20190124094332.33d5c67a@shemminger-XPS-13-9360>
On Wed, Jan 23, 2019 at 12:43 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 22 Jan 2019 22:41:45 -0800
> Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> > + if (tb[TCA_BASIC_PCNT]) {
> > + if (RTA_PAYLOAD(tb[TCA_BASIC_PCNT]) < sizeof(*pf)) {
> > + fprintf(f, "Broken perf counters\n");
> > + return -1;
>
> Errors should go to stderr not f (which is stdout).
Ok, I copy the code from u32 filter, so it has the same issue.
^ permalink raw reply
* Re: [Patch iproute2] tc: add hit counter for matchall
From: Cong Wang @ 2019-01-24 1:16 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Linux Kernel Network Developers, Martin Olsson, Jamal Hadi Salim,
Jiri Pirko, David Ahern
In-Reply-To: <20190124094706.57050b51@shemminger-XPS-13-9360>
On Wed, Jan 23, 2019 at 12:47 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Thu, 17 Jan 2019 13:18:55 -0800
> Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> >
> > + if (tb[TCA_MATCHALL_PCNT]) {
> > + if (RTA_PAYLOAD(tb[TCA_MATCHALL_PCNT]) < sizeof(*pf)) {
> > + print_string(PRINT_FP, NULL, "Broken perf counters\n", NULL);
>
> This the wrong way to print an error message with iproute2 suite.
> The correct answer is simple.
> fprintf(stderr, "Broken perf counters\n");
> and better yet give a more informative message that says what is incorrect.
What's your suggestion?
In fact, I am not sure if we really have to check the size here, it should
be safe to assume kernel will always dump a non-broken struct here,
and this struct will never shrink.
^ permalink raw reply
* Re: [PATCH bpf-next v5 00/12] bpf: dead code elimination
From: Alexei Starovoitov @ 2019-01-24 1:42 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: daniel, yhs, netdev, oss-drivers, kafai
In-Reply-To: <20190123064529.13518-1-jakub.kicinski@netronome.com>
On Tue, Jan 22, 2019 at 10:45:17PM -0800, Jakub Kicinski wrote:
> Hi!
>
> This set adds support for complete removal of dead code.
>
> Patch 3 contains all the code removal logic, patches 2 and 4
> additionally optimize branches around and to dead code.
>
> Patches 6 and 7 allow offload JITs to take advantage of the
> optimization. After a few small clean ups (8, 9, 10) nfp
> support is added (11, 12).
>
> Removing code directly in the verifier makes it easy to adjust
> the relevant metadata (line info, subprogram info). JITs for
> code store constrained architectures would have hard time
> performing such adjustments at JIT level. Removing subprograms
> or line info is very hard once BPF core finished the verification.
> For user space to perform dead code removal it would have to perform
> the execution simulation/analysis similar to what the verifier does.
>
> v3:
> - fix uninitilized var warning in GCC 6 (buildbot).
> v4:
> - simplify the linfo-keeping logic (Yonghong). Instead of
> trying to figure out that we are removing first instruction
> of a subprogram, just always keep last dead line info, if
> first live instruction doesn't have one.
> v5:
> - improve comments (Martin Lau).
The verifier is taking a page from the dragon book.
Applied to bpf-next.
Thanks!
^ permalink raw reply
* Re: [PATCH net] ax25: fix possible use-after-free
From: Eric Dumazet @ 2019-01-24 1:44 UTC (permalink / raw)
To: Cong Wang; +Cc: Eric Dumazet, David S . Miller, netdev, Ralf Baechle, syzbot
In-Reply-To: <CAM_iQpVW3qR3XyytQX5aSbU4CYfCB5N8g2syyDfz1NSdWG7ihg@mail.gmail.com>
On Wed, Jan 23, 2019 at 5:12 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Wed, Jan 23, 2019 at 3:42 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >
> >
> > On 01/23/2019 03:25 PM, Cong Wang wrote:
> > > On Tue, Jan 22, 2019 at 10:41 AM 'Eric Dumazet' via syzkaller
> > > <syzkaller@googlegroups.com> wrote:
> > >>
> > >> syzbot found that ax25 routes where not properly protected
> > >> against concurrent use [1].
> > >>
> > >> In this particular report the bug happened while
> > >> copying ax25->digipeat.
> > >>
> > >> Fix this problem by making sure we call ax25_get_route()
> > >> while ax25_route_lock is held, so that no modification
> > >> could happen while using the route.
> > >
> > > ax25_route_lock_use() is a read lock, so two ax25_rt_autobind()
> > > could still enter the same critical section?
> > >
> >
> > Not sure I understand your concern.
> >
> > The two ax25_rt_autobind() would only read the route contents,
> > so that should be fine ?
>
> Not sure if it is read-only and safe for the following code:
>
> if (ax25_rt->digipeat != NULL) {
> ax25->digipeat = kmemdup(ax25_rt->digipeat, sizeof(ax25_digi),
> GFP_ATOMIC);
ax25_rt would be the shared object. (protected by the read lock)
ax25 is private object in the thread/socket context.
So no worries.
> if (ax25->digipeat == NULL) {
> err = -ENOMEM;
> goto put;
> }
> ax25_adjust_path(addr, ax25->digipeat);
> }
>
> Maybe we leak memory here at least, not sure...
>
>
> >
> > >
> > >>
> > >> The current two ax25_get_route() callers do not sleep,
> > >> so this change should be fine.
> > >>
> > >> Once we do that, ax25_get_route() no longer needs to
> > >> grab a reference on the found route.
> > > .
> > >
> > > After your patch, ax25_hold_route() has no callers while
> > > ax25_put_route() still does. Is ->refcount always 1?
> >
> > Yes, the plan is to remove this refcount in net-next.
> >
>
> Good to know.
>
> Thanks.
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: suppress readelf stderr when probing for BTF support
From: Alexei Starovoitov @ 2019-01-24 1:45 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: netdev, davem, ast, daniel
In-Reply-To: <20190123172435.197862-1-sdf@google.com>
On Wed, Jan 23, 2019 at 09:24:35AM -0800, Stanislav Fomichev wrote:
> Before:
> $ make -s -C tools/testing/selftests/bpf
> readelf: Error: Missing knowledge of 32-bit reloc types used in DWARF
> sections of machine number 247
> readelf: Warning: unable to apply unsupported reloc type 10 to section
> .debug_info
> readelf: Warning: unable to apply unsupported reloc type 1 to section
> .debug_info
> readelf: Warning: unable to apply unsupported reloc type 10 to section
> .debug_info
>
> After:
> $ make -s -C tools/testing/selftests/bpf
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> tools/testing/selftests/bpf/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 70229de510f5..ff3cab5525c1 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -131,7 +131,7 @@ BTF_PAHOLE_PROBE := $(shell $(BTF_PAHOLE) --help 2>&1 | grep BTF)
> BTF_OBJCOPY_PROBE := $(shell $(LLVM_OBJCOPY) --help 2>&1 | grep -i 'usage.*llvm')
> BTF_LLVM_PROBE := $(shell echo "int main() { return 0; }" | \
> $(CLANG) -target bpf -O2 -g -c -x c - -o ./llvm_btf_verify.o; \
> - readelf -S ./llvm_btf_verify.o | grep BTF; \
> + readelf -S ./llvm_btf_verify.o 2>/dev/null | grep BTF; \
> /bin/rm -f ./llvm_btf_verify.o)
Silencing the warning may hide real issue later.
I'd rather fix whatever is necessary in readelf.
Does it use bfd? if so bpf support for bfd was practically complete.
^ permalink raw reply
* [PATCH net 1/1] MAINTAINERS: Update cavium networking drivers
From: Sudarsana Reddy Kalluru @ 2019-01-24 2:03 UTC (permalink / raw)
To: davem; +Cc: netdev, Ameen.Rahman
Following Marvell's acquisition of Cavium, we need to update all the
Cavium drivers maintainer's entries to point to our new e-mail addresses.
Signed-off-by: Sudarsana Reddy Kalluru <Sudarsana.Kalluru@cavium.com>
Signed-off-by: Ameen Rahman <Ameen.Rahman@cavium.com>
---
MAINTAINERS | 42 +++++++++++++++++++++---------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 51029a4..95be8f0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3052,8 +3052,8 @@ F: include/linux/bcm963xx_nvram.h
F: include/linux/bcm963xx_tag.h
BROADCOM BNX2 GIGABIT ETHERNET DRIVER
-M: Rasesh Mody <rasesh.mody@cavium.com>
-M: Dept-GELinuxNICDev@cavium.com
+M: Rasesh Mody <rmody@marvell.com>
+M: GR-Linux-NIC-Dev@marvell.com
L: netdev@vger.kernel.org
S: Supported
F: drivers/net/ethernet/broadcom/bnx2.*
@@ -3072,9 +3072,9 @@ S: Supported
F: drivers/scsi/bnx2i/
BROADCOM BNX2X 10 GIGABIT ETHERNET DRIVER
-M: Ariel Elior <ariel.elior@cavium.com>
-M: Sudarsana Kalluru <sudarsana.kalluru@cavium.com>
-M: everest-linux-l2@cavium.com
+M: Ariel Elior <aelior@marvell.com>
+M: Sudarsana Kalluru <skalluru@marvell.com>
+M: GR-everest-linux-l2@marvell.com
L: netdev@vger.kernel.org
S: Supported
F: drivers/net/ethernet/broadcom/bnx2x/
@@ -3249,9 +3249,9 @@ S: Supported
F: drivers/scsi/bfa/
BROCADE BNA 10 GIGABIT ETHERNET DRIVER
-M: Rasesh Mody <rasesh.mody@cavium.com>
-M: Sudarsana Kalluru <sudarsana.kalluru@cavium.com>
-M: Dept-GELinuxNICDev@cavium.com
+M: Rasesh Mody <rmody@marvell.com>
+M: Sudarsana Kalluru <skalluru@marvell.com>
+M: GR-Linux-NIC-Dev@marvell.com
L: netdev@vger.kernel.org
S: Supported
F: drivers/net/ethernet/brocade/bna/
@@ -10688,9 +10688,9 @@ S: Maintained
F: drivers/net/netdevsim/*
NETXEN (1/10) GbE SUPPORT
-M: Manish Chopra <manish.chopra@cavium.com>
-M: Rahul Verma <rahul.verma@cavium.com>
-M: Dept-GELinuxNICDev@cavium.com
+M: Manish Chopra <manishc@marvell.com>
+M: Rahul Verma <rahulv@marvell.com>
+M: GR-Linux-NIC-Dev@marvell.com
L: netdev@vger.kernel.org
S: Supported
F: drivers/net/ethernet/qlogic/netxen/
@@ -12474,8 +12474,8 @@ S: Supported
F: drivers/scsi/qedi/
QLOGIC QL4xxx ETHERNET DRIVER
-M: Ariel Elior <Ariel.Elior@cavium.com>
-M: everest-linux-l2@cavium.com
+M: Ariel Elior <aelior@marvell.com>
+M: GR-everest-linux-l2@marvell.com
L: netdev@vger.kernel.org
S: Supported
F: drivers/net/ethernet/qlogic/qed/
@@ -12483,8 +12483,8 @@ F: include/linux/qed/
F: drivers/net/ethernet/qlogic/qede/
QLOGIC QL4xxx RDMA DRIVER
-M: Michal Kalderon <Michal.Kalderon@cavium.com>
-M: Ariel Elior <Ariel.Elior@cavium.com>
+M: Michal Kalderon <mkalderon@marvell.com>
+M: Ariel Elior <aelior@marvell.com>
L: linux-rdma@vger.kernel.org
S: Supported
F: drivers/infiniband/hw/qedr/
@@ -12504,7 +12504,7 @@ F: Documentation/scsi/LICENSE.qla2xxx
F: drivers/scsi/qla2xxx/
QLOGIC QLA3XXX NETWORK DRIVER
-M: Dept-GELinuxNICDev@cavium.com
+M: GR-Linux-NIC-Dev@marvell.com
L: netdev@vger.kernel.org
S: Supported
F: Documentation/networking/device_drivers/qlogic/LICENSE.qla3xxx
@@ -12518,16 +12518,16 @@ F: Documentation/scsi/LICENSE.qla4xxx
F: drivers/scsi/qla4xxx/
QLOGIC QLCNIC (1/10)Gb ETHERNET DRIVER
-M: Shahed Shaikh <Shahed.Shaikh@cavium.com>
-M: Manish Chopra <manish.chopra@cavium.com>
-M: Dept-GELinuxNICDev@cavium.com
+M: Shahed Shaikh <shshaikh@marvell.com>
+M: Manish Chopra <manishc@marvell.com>
+M: GR-Linux-NIC-Dev@marvell.com
L: netdev@vger.kernel.org
S: Supported
F: drivers/net/ethernet/qlogic/qlcnic/
QLOGIC QLGE 10Gb ETHERNET DRIVER
-M: Manish Chopra <manish.chopra@cavium.com>
-M: Dept-GELinuxNICDev@cavium.com
+M: Manish Chopra <manishc@marvell.com>
+M: GR-Linux-NIC-Dev@marvell.com
L: netdev@vger.kernel.org
S: Supported
F: drivers/net/ethernet/qlogic/qlge/
--
1.8.3.1
^ permalink raw reply related
* [PATCH] tipc: remove dead code in struct tipc_topsrv
From: Zhaolong Zhang @ 2019-01-24 2:06 UTC (permalink / raw)
To: jon.maloy, ying.xue, davem, zhangzl2013
Cc: netdev, tipc-discussion, linux-kernel
max_rcvbuf_size is no longer used since commit "414574a0af36".
Signed-off-by: Zhaolong Zhang <zhangzl2013@126.com>
---
net/tipc/topsrv.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index efb16f6..4b1c083 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -60,7 +60,6 @@
* @awork: accept work item
* @rcv_wq: receive workqueue
* @send_wq: send workqueue
- * @max_rcvbuf_size: maximum permitted receive message length
* @listener: topsrv listener socket
* @name: server name
*/
@@ -72,7 +71,6 @@ struct tipc_topsrv {
struct work_struct awork;
struct workqueue_struct *rcv_wq;
struct workqueue_struct *send_wq;
- int max_rcvbuf_size;
struct socket *listener;
char name[TIPC_SERVER_NAME_LEN];
};
@@ -648,7 +646,6 @@ int tipc_topsrv_start(struct net *net)
return -ENOMEM;
srv->net = net;
- srv->max_rcvbuf_size = sizeof(struct tipc_subscr);
INIT_WORK(&srv->awork, tipc_topsrv_accept);
strscpy(srv->name, name, sizeof(srv->name));
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] netfilter: nat: Update comment of get_unique_tuple
From: YueHaibing @ 2019-01-24 2:18 UTC (permalink / raw)
To: pablo, kadlec, fw, davem; +Cc: linux-kernel, netdev, netfilter-devel, coreteam
In-Reply-To: <20190110130618.25344-1-yuehaibing@huawei.com>
ping...
On 2019/1/10 21:06, YueHaibing wrote:
> Replace outdated __ip_conntrack_confirm in comment.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> net/netfilter/nf_nat_core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> index d159e9e..f54a8db 100644
> --- a/net/netfilter/nf_nat_core.c
> +++ b/net/netfilter/nf_nat_core.c
> @@ -460,7 +460,8 @@ static void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
> * and NF_INET_LOCAL_OUT, we change the destination to map into the
> * range. It might not be possible to get a unique tuple, but we try.
> * At worst (or if we race), we will end up with a final duplicate in
> - * __ip_conntrack_confirm and drop the packet. */
> + * __nf_conntrack_confirm and drop the packet.
> + */
> static void
> get_unique_tuple(struct nf_conntrack_tuple *tuple,
> const struct nf_conntrack_tuple *orig_tuple,
>
^ permalink raw reply
* Re: [PATCH] tipc: remove dead code in struct tipc_topsrv
From: Ying Xue @ 2019-01-24 2:40 UTC (permalink / raw)
To: Zhaolong Zhang, jon.maloy, davem; +Cc: netdev, tipc-discussion, linux-kernel
In-Reply-To: <1548295601-23063-1-git-send-email-zhangzl2013@126.com>
On 1/24/19 10:06 AM, Zhaolong Zhang wrote:
> max_rcvbuf_size is no longer used since commit "414574a0af36".
>
> Signed-off-by: Zhaolong Zhang <zhangzl2013@126.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
> ---
> net/tipc/topsrv.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index efb16f6..4b1c083 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -60,7 +60,6 @@
> * @awork: accept work item
> * @rcv_wq: receive workqueue
> * @send_wq: send workqueue
> - * @max_rcvbuf_size: maximum permitted receive message length
> * @listener: topsrv listener socket
> * @name: server name
> */
> @@ -72,7 +71,6 @@ struct tipc_topsrv {
> struct work_struct awork;
> struct workqueue_struct *rcv_wq;
> struct workqueue_struct *send_wq;
> - int max_rcvbuf_size;
> struct socket *listener;
> char name[TIPC_SERVER_NAME_LEN];
> };
> @@ -648,7 +646,6 @@ int tipc_topsrv_start(struct net *net)
> return -ENOMEM;
>
> srv->net = net;
> - srv->max_rcvbuf_size = sizeof(struct tipc_subscr);
> INIT_WORK(&srv->awork, tipc_topsrv_accept);
>
> strscpy(srv->name, name, sizeof(srv->name));
>
^ permalink raw reply
* [PATCH bpf-next] bpftool: feature probing, change default action
From: Prashant Bhole @ 2019-01-24 2:51 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Prashant Bhole, netdev, Quentin Monnet, Jakub Kicinski
When 'bpftool feature' is executed it shows incorrect help string.
test# bpftool feature
Usage: bpftool bpftool probe [COMPONENT] [macros [prefix PREFIX]]
bpftool bpftool help
COMPONENT := { kernel | dev NAME }
Instead of fixing the help text by tweaking argv[] indices, this
patch changes the default action to 'probe'. It makes the behavior
consistent with other subcommands, where first subcommand without
extra parameter results in 'show' action.
Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
tools/bpf/bpftool/feature.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
index 993c6f1e5473..d672d9086fff 100644
--- a/tools/bpf/bpftool/feature.c
+++ b/tools/bpf/bpftool/feature.c
@@ -753,8 +753,8 @@ static int do_help(int argc, char **argv)
}
static const struct cmd cmds[] = {
- { "help", do_help },
{ "probe", do_probe },
+ { "help", do_help },
{ 0 }
};
--
2.20.1
^ permalink raw reply related
* [v2 PATCH] rhashtable: Still do rehash when we get EEXIST
From: Herbert Xu @ 2019-01-24 3:08 UTC (permalink / raw)
To: Josh Elsasser; +Cc: David S . Miller, josh, Thomas Graf, netdev, linux-kernel
In-Reply-To: <20190123211758.104275-1-jelsasser@appneta.com>
On Wed, Jan 23, 2019 at 01:17:58PM -0800, Josh Elsasser wrote:
> When running workloads with large bursts of fragmented packets, we've seen
> a few machines stuck returning -EEXIST from rht_shrink() and endlessly
> rescheduling their hash table's deferred work, pegging a CPU core.
>
> Root cause is commit da20420f83ea ("rhashtable: Add nested tables"), which
> stops ignoring the return code of rhashtable_shrink() and the reallocs
> used to grow the hashtable. This uncovers a bug in the shrink logic where
> "needs to shrink" check runs against the last table but the actual shrink
> operation runs on the first bucket_table in the hashtable (see below):
>
> +-------+ +--------------+ +---------------+
> | ht | | "first" tbl | | "last" tbl |
> | - tbl ---> | - future_tbl ---------> | - future_tbl ---> NULL
> +-------+ +--------------+ +---------------+
> ^^^ ^^^
> used by rhashtable_shrink() used by rht_shrink_below_30()
>
> A rehash then stalls out when both the last table needs to shrink, the
> first table has more elements than the target size, but rht_shrink() hits
> a non-NULL future_tbl and returns -EEXIST. This skips the item rehashing
> and kicks off a reschedule loop, as no forward progress can be made while
> the rhashtable needs to shrink.
>
> Extend rhashtable_shrink() with a "tbl" param to avoid endless exit-and-
> reschedules after hitting the EEXIST, allowing it to check a future_tbl
> pointer that can actually be non-NULL and make forward progress when the
> hashtable needs to shrink.
>
> Fixes: da20420f83ea ("rhashtable: Add nested tables")
> Signed-off-by: Josh Elsasser <jelsasser@appneta.com>
Thanks for catching this!
Although I think we should fix this in a different way. The problem
here is that the shrink cannot proceed because there was a previous
rehash that is still incomplete. We should wait for its completion
and then reattempt a shrinnk should it still be necessary.
So something like this:
---8<---
As it stands if a shrink is delayed because of an outstanding
rehash, we will go into a rescheduling loop without ever doing
the rehash.
This patch fixes this by still carrying out the rehash and then
rescheduling so that we can shrink after the completion of the
rehash should it still be necessary.
The return value of EEXIST captures this case and other cases
(e.g., another thread expanded/rehashed the table at the same
time) where we should still proceed with the rehash.
Fixes: da20420f83ea ("rhashtable: Add nested tables")
Reported-by: Josh Elsasser <jelsasser@appneta.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 852ffa5160f1..4edcf3310513 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -416,8 +416,12 @@ static void rht_deferred_worker(struct work_struct *work)
else if (tbl->nest)
err = rhashtable_rehash_alloc(ht, tbl, tbl->size);
- if (!err)
- err = rhashtable_rehash_table(ht);
+ if (!err || err == -EEXIST) {
+ int nerr;
+
+ nerr = rhashtable_rehash_table(ht);
+ err = err ?: nerr;
+ }
mutex_unlock(&ht->mutex);
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ 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