* Re: [PATCH net] net: mvneta: fix the remaining Rx descriptor unmapping issues
From: Gregory CLEMENT @ 2018-09-24 15:37 UTC (permalink / raw)
To: Antoine Tenart
Cc: davem, yelena, netdev, linux-kernel, thomas.petazzoni,
maxime.chevallier, miquel.raynal, nadavh, stefanc, ymarkman, mw
In-Reply-To: <20180924145613.27307-1-antoine.tenart@bootlin.com>
Hi Antoine,
On lun., sept. 24 2018, Antoine Tenart <antoine.tenart@bootlin.com> wrote:
> With CONFIG_DMA_API_DEBUG enabled we get DMA unmapping warning in
> various places of the mvneta driver, for example when putting down an
> interface while traffic is passing through.
>
> The issue is when using s/w buffer management, the Rx buffers are mapped
> using dma_map_page but unmapped with dma_unmap_single. This patch fixes
> this by using the right unmapping function.
>
> Fixes: 562e2f467e71 ("net: mvneta: Improve the buffer allocation method for SWBM")
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Given the clarification provided by Yelen, this patch looks good:
Reviewed-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Thanks,
Gregory
> ---
> drivers/net/ethernet/marvell/mvneta.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 2db9708f2e24..b4ed7d394d07 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -1890,8 +1890,8 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
> if (!data || !(rx_desc->buf_phys_addr))
> continue;
>
> - dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr,
> - MVNETA_RX_BUF_SIZE(pp->pkt_size), DMA_FROM_DEVICE);
> + dma_unmap_page(pp->dev->dev.parent, rx_desc->buf_phys_addr,
> + PAGE_SIZE, DMA_FROM_DEVICE);
> __free_page(data);
> }
> }
> @@ -2039,9 +2039,8 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
> frag_offset, frag_size,
> PAGE_SIZE);
>
> - dma_unmap_single(dev->dev.parent, phys_addr,
> - PAGE_SIZE,
> - DMA_FROM_DEVICE);
> + dma_unmap_page(dev->dev.parent, phys_addr,
> + PAGE_SIZE, DMA_FROM_DEVICE);
>
> rxq->left_size -= frag_size;
> }
> --
> 2.17.1
>
--
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com
^ permalink raw reply
* Re: KMSAN: uninit-value in memcmp (2)
From: Dmitry Vyukov @ 2018-09-24 9:39 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Vladis Dronov, syzbot, syzkaller-bugs, David Miller, Eric Dumazet,
LKML, Networking, sunlianwen
In-Reply-To: <CAG_fn=WwmJONmipmqdZkNj8K0Z-ZRmEiCvG4qt_fHknGYQe5Cg@mail.gmail.com>
On Mon, Sep 24, 2018 at 8:53 AM, Alexander Potapenko <glider@google.com> wrote:
> On Mon, Sep 24, 2018 at 12:09 AM Vladis Dronov <vdronov@redhat.com> wrote:
>>
>> Hello, Dmirty,
>>
>> Thank you for the reply. Can we please, discuss this further?
> Hi Vladis,
>> > You can see on dashboard that the last crash
>> > for the second version (2) happened just few days ago. So this is a
>> > different bug.
> FWIW I've just double-checked that the reproducer provided by
> syzkaller in the original message still triggers the report from the
> original message in the latest KMSAN tree (which already contains the
> __hw_addr_add_ex() fix from April).
>> Well... yes and no. When I was looking at this bug (bug?id=088efeac32fd) I was looking
>> at the report at "2018/05/09 18:55" (https://syzkaller.appspot.com/text?tag=CrashReport&x=141b707b800000),
>> since it was the only report with a reproducer. This was my error.
>>
>> The error and the call trace in this report are:
>>
>> >>>
>> BUG: KMSAN: uninit-value in memcmp+0x119/0x180 lib/string.c:861
>> CPU: 0 PID: 38 Comm: kworker/0:1 Not tainted 4.17.0-rc3+ #88
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>> Workqueue: ipv6_addrconf addrconf_dad_work
>> Call Trace:
>> __dump_stack lib/dump_stack.c:77 [inline]
>> dump_stack+0x185/0x1d0 lib/dump_stack.c:113
>> kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067
>> __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683
>> memcmp+0x119/0x180 lib/string.c:861
>> __hw_addr_add_ex net/core/dev_addr_lists.c:61 [inline]
>> __dev_mc_add+0x1fc/0x900 net/core/dev_addr_lists.c:670
>> dev_mc_add+0x6d/0x80 net/core/dev_addr_lists.c:687
>> igmp6_group_added+0x2db/0xa00 net/ipv6/mcast.c:662
>> ipv6_dev_mc_inc+0xe9e/0x1130 net/ipv6/mcast.c:914
>> addrconf_join_solict net/ipv6/addrconf.c:2103 [inline]
>> addrconf_dad_begin net/ipv6/addrconf.c:3853 [inline]
>> addrconf_dad_work+0x462/0x2a20 net/ipv6/addrconf.c:3979
>> process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145
>> worker_thread+0x113c/0x24f0 kernel/workqueue.c:2279
>> kthread+0x539/0x720 kernel/kthread.c:239
>> ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:412
>>
>> Local variable description: ----buf@igmp6_group_added
>> Variable was created at:
>> igmp6_group_added+0x4a/0xa00 net/ipv6/mcast.c:650
>> ipv6_dev_mc_inc+0xe9e/0x1130 net/ipv6/mcast.c:914
>> <<<
>>
>> It is the same like in bug?id=3887c0d99aecb27d085180c5222d245d08a30806
>> which, after some more test, made me believe these bugs are duplicate
>> and are fixed by the same commit.
>>
>> But let's look at another report at "2018/09/12 21:00"
>> (https://syzkaller.appspot.com/text?tag=CrashReport&x=14f99b71400000)
>> at the bug (bug?id=088efeac32fd), the one you've mentioned as
>> "the last crash for the second version (2) happened just few days ago".
>>
>> Its error and the call trace are completely different:
>>
>> >>>
>> BUG: KMSAN: uninit-value in memcmp+0x11d/0x180 lib/string.c:863
>> CPU: 0 PID: 6107 Comm: syz-executor4 Not tainted 4.19.0-rc3+ #45
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>> Call Trace:
>> __dump_stack lib/dump_stack.c:77 [inline]
>> dump_stack+0x14b/0x190 lib/dump_stack.c:113
>> kmsan_report+0x183/0x2b0 mm/kmsan/kmsan.c:956
>> __msan_warning+0x70/0xc0 mm/kmsan/kmsan_instr.c:645
>> memcmp+0x11d/0x180 lib/string.c:863
>> dev_uc_add_excl+0x165/0x7b0 net/core/dev_addr_lists.c:464
>> ndo_dflt_fdb_add net/core/rtnetlink.c:3463 [inline]
>> rtnl_fdb_add+0x1081/0x1270 net/core/rtnetlink.c:3558
>> rtnetlink_rcv_msg+0xa0b/0x1530 net/core/rtnetlink.c:4715
>> netlink_rcv_skb+0x36e/0x5f0 net/netlink/af_netlink.c:2454
>> rtnetlink_rcv+0x50/0x60 net/core/rtnetlink.c:4733
>> netlink_unicast_kernel net/netlink/af_netlink.c:1317 [inline]
>> netlink_unicast+0x1638/0x1720 net/netlink/af_netlink.c:1343
>> netlink_sendmsg+0x1205/0x1290 net/netlink/af_netlink.c:1908
>> sock_sendmsg_nosec net/socket.c:621 [inline]
>> sock_sendmsg net/socket.c:631 [inline]
>> ...
>> Uninit was created at:
>> ...
>> slab_post_alloc_hook mm/slab.h:446 [inline]
>> slab_alloc_node mm/slub.c:2718 [inline]
>> __kmalloc_node_track_caller+0x9e7/0x1160 mm/slub.c:4351
>> __kmalloc_reserve net/core/skbuff.c:138 [inline]
>> __alloc_skb+0x2f5/0x9e0 net/core/skbuff.c:206
>> alloc_skb include/linux/skbuff.h:996 [inline]
>> netlink_alloc_large_skb net/netlink/af_netlink.c:1189 [inline]
>> netlink_sendmsg+0xb49/0x1290 net/netlink/af_netlink.c:1883
>> sock_sendmsg_nosec net/socket.c:621 [inline]
>> sock_sendmsg net/socket.c:631 [inline]
>> ___sys_sendmsg+0xe70/0x1290 net/socket.c:2114
>> <<<
>>
>> This is a different bug. How come these 2 different reports for 2 different
>> bugs have ended in the same syzkaller report (bug?id=088efeac32fd) ?
>
> I suspect this is because syzbot used the top stack frame as the
> report signature.
> There's a mechanism to ignore frames like memcmp() in the reports, not
> sure why didn't it work in this case (maybe it just wasn't in place at
> the time the report happened).
>> One bug is fixed by the "net: fix uninit-value in __hw_addr_add_ex()" commit,
>> the second one is not, but they are still in the same syzkaller report.
>>
>> This was the reason of my confusion. I'm not sure how to fix this. If it is possible,
>> probably we need to cancel/revoke "#syz fix: net: fix uninit-value in __hw_addr_add_ex()"
>> for this syzkaller report (bug?id=088efeac32fd). And then "split" it into 2 or
>> more different reports, but I'm not sure if this is possible.
>>
>> Probably, syzkaller needs to look deeper into the KMSAN reports to differentiate
>> KMSAN errors happening because of different reasons.
>>
>> > On Sun, Sep 23, 2018 at 6:02 PM, Vladis Dronov <vdronov@redhat.com> wrote:
>> > > #syz fix: net: fix uninit-value in __hw_addr_add_ex()
>> >
>> > Hi Vladis,
>> >
>> > This can be fixed with "net: fix uninit-value in __hw_addr_add_ex()".
>> > That commit landed in April, syzbot waited till the commit reached all
>> > tested trees, and then closed the bug.
>> > But the similar bug continued to happen, so syzbot created second
>> > version of this bug (2). You can see on dashboard that the last crash
>> > for the second version (2) happened just few days ago. So this is a
>> > different bug.
Precisely discriminating bugs (root causes) bases on crash text is
generally undecidable problem, even for humans. We even can have
literally equal crash texts, which are still different bugs. And we
can have significantly differently looking crash texts, which are
actually caused by the same root cause. syzbot extracts some
"identity" string for each crash and than uses that string to
discriminate crashes and sort them into bins. This identity string is
what you see in email subject and bug title on dashboard. This method
can have both false positives and false negatives, but works
reasonably well in most cases and looks like the best practical
option.
For this exact instance (memcmp) we actually improved the analysis
logic recently:
https://github.com/google/syzkaller/commit/0e29942f77715486995d996f80f82742812d75a2#diff-abe1515f011fad2659ff218f9eea9ae1
But this crash was analyzed and reported before the change. So if this
crash happens again it should be reported as "in __hw_addr_add_ex"
now.
Re __hw_addr_add_ex bug, as Alex noted the crash was detected _after_
the fixing commit went in. So it's something new and different and
can't be fixed by the older commit.
There are no general, single guideline as to what to do when several
different bugs glued together into a single bug. Fixing at least one
of them (any) in the context of the bug is good, fixing both is good
too. When/if a bug is closed, new occurrences of similar crashes (the
same identity string) will lead to creation of a new bug. So if we fix
only one and close the bug, eventually the second one will lead to a
new bug (won't be lost), now dedicated to this second crash.
Now syzbot thinks that this bug is fixed/closed:
https://syzkaller.appspot.com/bug?extid=d3402c47f680ff24b29c
There is specifically no "undo" functionality, because it's inherently
racy with creation of a new version of this bugs by new crashes. So if
of these crashes will happen again, syzbot will open new bugs (now
with better discriminated titles). We can wait for that. Or we can
submit new fixes without waiting for new syzbot bugs (adding
Reported-by to new commits referencing this bug should not do any
harm).
Hope this clarifies things a bit.
Thanks
^ permalink raw reply
* Re: [PATCH 0/2] gpiolib: Fix issues introduced by fast bitmap processing path
From: Marek Szyprowski @ 2018-09-24 9:43 UTC (permalink / raw)
To: Janusz Krzysztofik, Linus Walleij
Cc: Jonathan Corbet, Miguel Ojeda Sandonis, Peter Korsgaard,
Peter Rosin, Ulf Hansson, Andrew Lunn, Florian Fainelli,
David S. Miller, Dominik Brodowski, Greg Kroah-Hartman,
Kishon Vijay Abraham I, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
Jiri Slaby, Willy Tarreau, Geert Uytterhoeven
In-Reply-To: <20180923235336.22148-1-jmkrzyszt@gmail.com>
Hi Janusz,
On 2018-09-24 01:53, Janusz Krzysztofik wrote:
> While investigating possible reasons of GPIO fast bitmap processing
> related boot hang on Samsung Snow Chromebook, reported by Marek
> Szyprowski (thanks!), I've discovered one coding bug, addressed by
> PATCH 1/2 of this series, and one potential regression introduced at
> design level of the solution, hopefully fixed by PATCH 2/2. See
> commit messages for details.
>
> Janusz Krzysztofik (2):
> gpiolib: Fix missing updates of bitmap index
> gpiolib: Fix array members of same chip processed separately
>
> The fixes should resolve the boot hang observed by Marek, however the
> second change excludes that particular case from fast bitmap processing
> and restores the old behaviour.
I confirm, that the above 2 patches fixes boot issue on Samsung Snow
Chromebook with next-20180920.
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Hence, it is possible still another
> issue which have had an influence on that boot hang exists in the code.
> In order to fully verify the fix, it would have to be tested on a
> platform where an array of GPIO descriptors is used which starts from
> at least two consecutive pins of one GPIO chip in hardware order,
> starting ftom 0, followed by one or more pins belonging to other
> chip(s).
>
> In order to verify if separate calls to .set() chip callback for each
> pin instead of one call to .set_multiple() is actually the reason of
> boot hang on Samsung Snow Chromebook, the affected driver -
> drivers/mmc/core/pwrseq_simple.c - would have to be temporarily
> modified for testing purposes so it calls gpiod_set_value() for each
> pin instead of gpiod_set_array_value() for all of them. If that would
> also result in boot hang, we could be sure the issue was really the
> one addressed by the second fix. Marek, could you please try to
> perform such test?
Yes, I've just tested next-20180920 only with the first patch from this
patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c.
It boots fine, so indeed the issue is in handling of arrays of gpios.
Just to be sure I did it right, this is my change to the mentioned file:
diff --git a/drivers/mmc/core/pwrseq_simple.c
b/drivers/mmc/core/pwrseq_simple.c
index 7f882a2bb872..9397dc1f2e38 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct
mmc_pwrseq_simple *pwrseq,
int value)
{
struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
+ int i;
- if (!IS_ERR(reset_gpios)) {
- DECLARE_BITMAP(values, BITS_PER_TYPE(value));
- int nvalues = reset_gpios->ndescs;
-
- values[0] = value;
-
- gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
- reset_gpios->info, values);
- }
+ if (!IS_ERR(reset_gpios))
+ for (i = 0; i < reset_gpios->ndescs; i++)
+ gpiod_set_value_cansleep(reset_gpios->desc[i], value);
}
static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply related
* [PATCH] qed: fix spelling mistake "toogle" -> "toggle"
From: Colin King @ 2018-09-24 15:51 UTC (permalink / raw)
To: Ariel Elior, everest-linux-l2, David S . Miller, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Trivial fix to spelling mistake in DP_VERBOSE message
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
index be941cfaa2d4..c71391b9c757 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
@@ -228,7 +228,7 @@ static int qed_rdma_alloc(struct qed_hwfn *p_hwfn,
num_cons, "Toggle");
if (rc) {
DP_VERBOSE(p_hwfn, QED_MSG_RDMA,
- "Failed to allocate toogle bits, rc = %d\n", rc);
+ "Failed to allocate toggle bits, rc = %d\n", rc);
goto free_cq_map;
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next] net/tls: Fixed race condition in async encryption
From: Vakul Garg @ 2018-09-24 10:05 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, doronrk, Vakul Garg
On processors with multi-engine crypto accelerators, it is possible that
multiple records get encrypted in parallel and their encryption
completion is notified to different cpus in multicore processor. This
leads to the situation where tls_encrypt_done() starts executing in
parallel on different cores. In current implementation, encrypted
records are queued to tx_ready_list in tls_encrypt_done(). This requires
addition to linked list 'tx_ready_list' to be protected. As
tls_decrypt_done() could be executing in irq content, it is not possible
to protect linked list addition operation using a lock.
To fix the problem, we remove linked list addition operation from the
irq context. We do tx_ready_list addition/removal operation from
application context only and get rid of possible multiple access to
the linked list. Before starting encryption on the record, we add it to
the tail of tx_ready_list. To prevent tls_tx_records() from transmitting
it, we mark the record with a new flag 'tx_ready' in 'struct tls_rec'.
When record encryption gets completed, tls_encrypt_done() has to only
update the 'tx_ready' flag to true & linked list add operation is not
required.
The changed logic brings some other side benefits. Since the records
are always submitted in tls sequence number order for encryption, the
tx_ready_list always remains sorted and addition of new records to it
does not have to traverse the linked list.
Lastly, we renamed tx_ready_list in 'struct tls_sw_context_tx' to
'tx_list'. This is because now, the some of the records at the tail are
not ready to transmit.
Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption")
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
include/net/tls.h | 16 ++++-------
net/tls/tls_main.c | 4 +--
net/tls/tls_sw.c | 81 ++++++++++++++++++++----------------------------------
3 files changed, 37 insertions(+), 64 deletions(-)
diff --git a/include/net/tls.h b/include/net/tls.h
index 3aa73e2d8823..1615fb5ea114 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -99,6 +99,7 @@ enum {
*/
struct tls_rec {
struct list_head list;
+ int tx_ready;
int tx_flags;
struct scatterlist sg_plaintext_data[MAX_SKB_FRAGS];
struct scatterlist sg_encrypted_data[MAX_SKB_FRAGS];
@@ -128,7 +129,7 @@ struct tls_sw_context_tx {
struct crypto_wait async_wait;
struct tx_work tx_work;
struct tls_rec *open_rec;
- struct list_head tx_ready_list;
+ struct list_head tx_list;
atomic_t encrypt_pending;
int async_notify;
@@ -220,7 +221,6 @@ struct tls_context {
struct scatterlist *partially_sent_record;
u16 partially_sent_offset;
- u64 tx_seq_number; /* Next TLS seqnum to be transmitted */
unsigned long flags;
bool in_tcp_sendpages;
@@ -341,21 +341,15 @@ static inline bool tls_is_pending_open_record(struct tls_context *tls_ctx)
return tls_ctx->pending_open_record_frags;
}
-static inline bool is_tx_ready(struct tls_context *tls_ctx,
- struct tls_sw_context_tx *ctx)
+static inline bool is_tx_ready(struct tls_sw_context_tx *ctx)
{
struct tls_rec *rec;
- u64 seq;
- rec = list_first_entry(&ctx->tx_ready_list, struct tls_rec, list);
+ rec = list_first_entry(&ctx->tx_list, struct tls_rec, list);
if (!rec)
return false;
- seq = be64_to_cpup((const __be64 *)&rec->aad_space);
- if (seq == tls_ctx->tx_seq_number)
- return true;
- else
- return false;
+ return READ_ONCE(rec->tx_ready);
}
struct sk_buff *
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 06094de7a3d9..b428069a1b05 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -212,7 +212,7 @@ int tls_push_pending_closed_record(struct sock *sk,
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
if (tls_is_partially_sent_record(tls_ctx) ||
- !list_empty(&ctx->tx_ready_list))
+ !list_empty(&ctx->tx_list))
return tls_tx_records(sk, flags);
else
return tls_ctx->push_pending_record(sk, flags);
@@ -233,7 +233,7 @@ static void tls_write_space(struct sock *sk)
}
/* Schedule the transmission if tx list is ready */
- if (is_tx_ready(ctx, tx_ctx) && !sk->sk_write_pending) {
+ if (is_tx_ready(tx_ctx) && !sk->sk_write_pending) {
/* Schedule the transmission */
if (!test_and_set_bit(BIT_TX_SCHEDULED, &tx_ctx->tx_bitmask))
schedule_delayed_work(&tx_ctx->tx_work.work, 0);
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index bcb24c498b84..d30d65bf0753 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -329,29 +329,6 @@ static void tls_free_both_sg(struct sock *sk)
&rec->sg_plaintext_size);
}
-static bool append_tx_ready_list(struct tls_context *tls_ctx,
- struct tls_sw_context_tx *ctx,
- struct tls_rec *enc_rec)
-{
- u64 new_seq = be64_to_cpup((const __be64 *)&enc_rec->aad_space);
- struct list_head *pos;
-
- /* Need to insert encrypted record in tx_ready_list sorted
- * as per sequence number. Traverse linked list from tail.
- */
- list_for_each_prev(pos, &ctx->tx_ready_list) {
- struct tls_rec *rec = (struct tls_rec *)pos;
- u64 seq = be64_to_cpup((const __be64 *)&rec->aad_space);
-
- if (new_seq > seq)
- break;
- }
-
- list_add((struct list_head *)&enc_rec->list, pos);
-
- return is_tx_ready(tls_ctx, ctx);
-}
-
int tls_tx_records(struct sock *sk, int flags)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
@@ -360,7 +337,7 @@ int tls_tx_records(struct sock *sk, int flags)
int tx_flags, rc = 0;
if (tls_is_partially_sent_record(tls_ctx)) {
- rec = list_first_entry(&ctx->tx_ready_list,
+ rec = list_first_entry(&ctx->tx_list,
struct tls_rec, list);
if (flags == -1)
@@ -373,18 +350,15 @@ int tls_tx_records(struct sock *sk, int flags)
goto tx_err;
/* Full record has been transmitted.
- * Remove the head of tx_ready_list
+ * Remove the head of tx_list
*/
- tls_ctx->tx_seq_number++;
list_del(&rec->list);
kfree(rec);
}
- /* Tx all ready records which have expected sequence number */
- list_for_each_entry_safe(rec, tmp, &ctx->tx_ready_list, list) {
- u64 seq = be64_to_cpup((const __be64 *)&rec->aad_space);
-
- if (seq == tls_ctx->tx_seq_number) {
+ /* Tx all ready records */
+ list_for_each_entry_safe(rec, tmp, &ctx->tx_list, list) {
+ if (READ_ONCE(rec->tx_ready)) {
if (flags == -1)
tx_flags = rec->tx_flags;
else
@@ -396,7 +370,6 @@ int tls_tx_records(struct sock *sk, int flags)
if (rc)
goto tx_err;
- tls_ctx->tx_seq_number++;
list_del(&rec->list);
kfree(rec);
} else {
@@ -446,9 +419,18 @@ static void tls_encrypt_done(struct crypto_async_request *req, int err)
}
}
- /* Append the record in tx queue */
- if (rec)
- ready = append_tx_ready_list(tls_ctx, ctx, rec);
+ if (rec) {
+ struct tls_rec *first_rec;
+
+ /* Mark the record as ready for transmission */
+ smp_store_mb(rec->tx_ready, true);
+
+ /* If received record is at head of tx_list, schedule tx */
+ first_rec = list_first_entry(&ctx->tx_list,
+ struct tls_rec, list);
+ if (rec == first_rec)
+ ready = true;
+ }
pending = atomic_dec_return(&ctx->encrypt_pending);
@@ -484,6 +466,8 @@ static int tls_do_encryption(struct sock *sk,
aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_BACKLOG,
tls_encrypt_done, sk);
+ /* Add the record in tx_list */
+ list_add_tail((struct list_head *)&rec->list, &ctx->tx_list);
atomic_inc(&ctx->encrypt_pending);
rc = crypto_aead_encrypt(aead_req);
@@ -493,9 +477,12 @@ static int tls_do_encryption(struct sock *sk,
rec->sg_encrypted_data[0].length += tls_ctx->tx.prepend_size;
}
- /* Case of encryption failure */
- if (rc && rc != -EINPROGRESS)
+ if (!rc) {
+ WRITE_ONCE(rec->tx_ready, true);
+ } else if (rc != -EINPROGRESS) {
+ list_del(&rec->list);
return rc;
+ }
/* Unhook the record from context if encryption is not failure */
ctx->open_rec = NULL;
@@ -544,13 +531,7 @@ static int tls_push_record(struct sock *sk, int flags,
return rc;
}
- /* Put the record in tx_ready_list and start tx if permitted.
- * This happens only when encryption is not asynchronous.
- */
- if (append_tx_ready_list(tls_ctx, ctx, rec))
- return tls_tx_records(sk, flags);
-
- return 0;
+ return tls_tx_records(sk, flags);
}
static int tls_sw_push_pending_record(struct sock *sk, int flags)
@@ -1566,7 +1547,7 @@ void tls_sw_free_resources_tx(struct sock *sk)
/* Tx whatever records we can transmit and abandon the rest */
tls_tx_records(sk, -1);
- /* Free up un-sent records in tx_ready_list. First, free
+ /* Free up un-sent records in tx_list. First, free
* the partially sent record if any at head of tx_list.
*/
if (tls_ctx->partially_sent_record) {
@@ -1583,13 +1564,13 @@ void tls_sw_free_resources_tx(struct sock *sk)
tls_ctx->partially_sent_record = NULL;
- rec = list_first_entry(&ctx->tx_ready_list,
+ rec = list_first_entry(&ctx->tx_list,
struct tls_rec, list);
list_del(&rec->list);
kfree(rec);
}
- list_for_each_entry_safe(rec, tmp, &ctx->tx_ready_list, list) {
+ list_for_each_entry_safe(rec, tmp, &ctx->tx_list, list) {
free_sg(sk, rec->sg_encrypted_data,
&rec->sg_encrypted_num_elem,
&rec->sg_encrypted_size);
@@ -1633,7 +1614,7 @@ void tls_sw_free_resources_rx(struct sock *sk)
kfree(ctx);
}
-/* The work handler to transmitt the encrypted records in tx_ready_list */
+/* The work handler to transmitt the encrypted records in tx_list */
static void tx_work_handler(struct work_struct *work)
{
struct delayed_work *delayed_work = to_delayed_work(work);
@@ -1700,7 +1681,7 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
crypto_info = &ctx->crypto_send.info;
cctx = &ctx->tx;
aead = &sw_ctx_tx->aead_send;
- INIT_LIST_HEAD(&sw_ctx_tx->tx_ready_list);
+ INIT_LIST_HEAD(&sw_ctx_tx->tx_list);
INIT_DELAYED_WORK(&sw_ctx_tx->tx_work.work, tx_work_handler);
sw_ctx_tx->tx_work.sk = sk;
} else {
@@ -1789,8 +1770,6 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
sw_ctx_rx->sk_poll = sk->sk_socket->ops->poll;
strp_check_rcv(&sw_ctx_rx->strp);
- } else {
- ctx->tx_seq_number = be64_to_cpup((const __be64 *)rec_seq);
}
goto out;
--
2.13.6
^ permalink raw reply related
* [PATCH net-next] tls: Fixed uninitialised vars warning
From: Vakul Garg @ 2018-09-24 10:39 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, doronrk, Vakul Garg
In tls_sw_sendmsg() and tls_sw_sendpage(), it is possible that the
uninitialised variable 'ret' gets passed to sk_stream_error(). So
initialise local variable 'ret' to '0. The warnings were detected by
'smatch' tool.
Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption")
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
net/tls/tls_sw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index bcb24c498b84..102d84bdb2ab 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -709,7 +709,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
int record_room;
int num_zc = 0;
int orig_size;
- int ret;
+ int ret = 0;
if (msg->msg_flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL))
return -ENOTSUPP;
@@ -901,8 +901,8 @@ int tls_sw_sendpage(struct sock *sk, struct page *page,
int num_async = 0;
bool full_record;
int record_room;
+ int ret = 0;
bool eor;
- int ret;
if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
MSG_SENDPAGE_NOTLAST))
--
2.13.6
^ permalink raw reply related
* Re: [PATCH net-next 0/2] net: mvpp2: Add txq to CPU mapping
From: David Miller @ 2018-09-24 17:01 UTC (permalink / raw)
To: maxime.chevallier
Cc: netdev, linux-kernel, antoine.tenart, thomas.petazzoni,
gregory.clement, miquel.raynal, nadavh, stefanc, ymarkman, mw
In-Reply-To: <20180924091106.15094-1-maxime.chevallier@bootlin.com>
From: Maxime Chevallier <maxime.chevallier@bootlin.com>
Date: Mon, 24 Sep 2018 11:11:04 +0200
> This short series adds XPS support to the mvpp2 driver, by mapping
> txqs and CPUs. This comes with a patch using round-robin scheduling
> for the HW to pick the next txq to transmit from, instead of the default
> fixed-priority scheduling.
Series applied, thanks Maxime.
^ permalink raw reply
* Re: [PATCH net] ip6_tunnel: be careful when accessing the inner header
From: Paolo Abeni @ 2018-09-24 11:03 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David Miller,
Alexander Potapenko
In-Reply-To: <CAM_iQpX5ySGZqKTsaQO6WHaFLMqG6Q2ZAUT=ctGijL-7KU_1uA@mail.gmail.com>
On Fri, 2018-09-21 at 11:51 -0700, Cong Wang wrote:
> On Wed, Sep 19, 2018 at 6:04 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> > index 419960b0ba16..a0b6932c3afd 100644
> > --- a/net/ipv6/ip6_tunnel.c
> > +++ b/net/ipv6/ip6_tunnel.c
> > @@ -1234,7 +1234,7 @@ static inline int
> > ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
> > {
> > struct ip6_tnl *t = netdev_priv(dev);
> > - const struct iphdr *iph = ip_hdr(skb);
> > + const struct iphdr *iph;
> > int encap_limit = -1;
> > struct flowi6 fl6;
> > __u8 dsfield;
> > @@ -1242,6 +1242,11 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
> > u8 tproto;
> > int err;
> >
> > + /* ensure we can access the full inner ip header */
> > + if (!pskb_may_pull(skb, sizeof(struct iphdr)))
> > + return -1;
> > +
> > + iph = ip_hdr(skb);
>
> Hmm...
>
> How do IPv4 tunnels ensure they have the right inner header to access?
> ip_tunnel_xmit() uses skb_inner_network_header() to access inner header
> which doesn't have any check either AFAIK.
You are right, I think we need similar checks for ip_tunnel_xmit(),
too.
I'll try to cook a patch.
Cheers,
Paolo
^ permalink raw reply
* Re: [PATCH net-next] mlxsw: Make MLXSW_SP1_FWREV_MINOR a hard requirement
From: Petr Machata @ 2018-09-24 11:14 UTC (permalink / raw)
To: Ido Schimmel; +Cc: Andrew Lunn, Ido Schimmel, netdev, davem, jiri, mlxsw
In-Reply-To: <20180923222839.GA26312@splinter>
Ido Schimmel <idosch@idosch.org> writes:
> On Mon, Sep 24, 2018 at 12:04:17AM +0200, Andrew Lunn wrote:
>> But doesn't that mean you reflash the device with the minimum version,
>> when in fact there could be a much newer version in /lib/firmware?
>
> No, because we always enforce the latest version we post to
> linux-firmware. We try to keep firmware updates at a minimum, so if we
> decided to post a new version it's either because the driver now
> requires a feature from this version (which makes older versions
> incompatible) or because a critical bug was fixed in that version.
If you consider the case of an older driver and a recent FW update that
the driver didn't know about, then yes, such FW would be ignored. You
can flash it by hand of course (ethtool -f), as long as it's on the
right branch.
Petr
^ permalink raw reply
* [PATCH] wimax/i2400m: fix spelling mistake "unitialized" -> "uninitialized"
From: Colin King @ 2018-09-24 17:18 UTC (permalink / raw)
To: Inaky Perez-Gonzalez, linux-wimax, David S . Miller, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Trivial fix to spelling mistake in ms_to_errno array of error messages
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wimax/i2400m/control.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wimax/i2400m/control.c b/drivers/net/wimax/i2400m/control.c
index 094cea775d0c..0475ad553707 100644
--- a/drivers/net/wimax/i2400m/control.c
+++ b/drivers/net/wimax/i2400m/control.c
@@ -257,7 +257,7 @@ static const struct
[I2400M_MS_ACCESSIBILITY_ERROR] = { "accesibility error", -EIO },
[I2400M_MS_BUSY] = { "busy", -EBUSY },
[I2400M_MS_CORRUPTED_TLV] = { "corrupted TLV", -EILSEQ },
- [I2400M_MS_UNINITIALIZED] = { "not unitialized", -EILSEQ },
+ [I2400M_MS_UNINITIALIZED] = { "not uninitialized", -EILSEQ },
[I2400M_MS_UNKNOWN_ERROR] = { "unknown error", -EIO },
[I2400M_MS_PRODUCTION_ERROR] = { "production error", -EIO },
[I2400M_MS_NO_RF] = { "no RF", -EIO },
--
2.17.1
^ permalink raw reply related
* [PATCH] net: qca_spi: Introduce write register verification
From: Stefan Wahren @ 2018-09-24 11:20 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-kernel, Stefan Wahren, Michael Heimpold
The SPI protocol for the QCA7000 doesn't have any fault detection.
In order to increase the drivers reliability in noisy environments,
we could implement a write verification inspired by the enc28j60.
This should avoid situations were the driver wrongly assumes the
receive interrupt is enabled and miss all incoming packets.
This function is disabled per default and can be controlled via module
parameter wr_verify.
Signed-off-by: Michael Heimpold <michael.heimpold@i2se.com>
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/net/ethernet/qualcomm/qca_7k.c | 34 +++++++++++++++++++++++++++++--
drivers/net/ethernet/qualcomm/qca_7k.h | 2 +-
drivers/net/ethernet/qualcomm/qca_debug.c | 1 +
drivers/net/ethernet/qualcomm/qca_spi.c | 28 ++++++++++++++++++-------
drivers/net/ethernet/qualcomm/qca_spi.h | 1 +
5 files changed, 56 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/qualcomm/qca_7k.c b/drivers/net/ethernet/qualcomm/qca_7k.c
index 6c8543f..4292c89 100644
--- a/drivers/net/ethernet/qualcomm/qca_7k.c
+++ b/drivers/net/ethernet/qualcomm/qca_7k.c
@@ -81,8 +81,8 @@ qcaspi_read_register(struct qcaspi *qca, u16 reg, u16 *result)
return ret;
}
-int
-qcaspi_write_register(struct qcaspi *qca, u16 reg, u16 value)
+static int
+__qcaspi_write_register(struct qcaspi *qca, u16 reg, u16 value)
{
__be16 tx_data[2];
struct spi_transfer transfer[2];
@@ -117,3 +117,33 @@ qcaspi_write_register(struct qcaspi *qca, u16 reg, u16 value)
return ret;
}
+
+int
+qcaspi_write_register(struct qcaspi *qca, u16 reg, u16 value, int retry)
+{
+ int ret, i = 0;
+ u16 confirmed;
+
+ do {
+ ret = __qcaspi_write_register(qca, reg, value);
+ if (ret)
+ return ret;
+
+ if (!retry)
+ return 0;
+
+ ret = qcaspi_read_register(qca, reg, &confirmed);
+ if (ret)
+ return ret;
+
+ ret = confirmed != value;
+ if (!ret)
+ return 0;
+
+ i++;
+ qca->stats.write_verify_failed++;
+
+ } while (i <= retry);
+
+ return ret;
+}
diff --git a/drivers/net/ethernet/qualcomm/qca_7k.h b/drivers/net/ethernet/qualcomm/qca_7k.h
index 27124c2..356de8e 100644
--- a/drivers/net/ethernet/qualcomm/qca_7k.h
+++ b/drivers/net/ethernet/qualcomm/qca_7k.h
@@ -66,6 +66,6 @@
void qcaspi_spi_error(struct qcaspi *qca);
int qcaspi_read_register(struct qcaspi *qca, u16 reg, u16 *result);
-int qcaspi_write_register(struct qcaspi *qca, u16 reg, u16 value);
+int qcaspi_write_register(struct qcaspi *qca, u16 reg, u16 value, int retry);
#endif /* _QCA_7K_H */
diff --git a/drivers/net/ethernet/qualcomm/qca_debug.c b/drivers/net/ethernet/qualcomm/qca_debug.c
index 51d89c8..a9f1bc0 100644
--- a/drivers/net/ethernet/qualcomm/qca_debug.c
+++ b/drivers/net/ethernet/qualcomm/qca_debug.c
@@ -60,6 +60,7 @@ static const char qcaspi_gstrings_stats[][ETH_GSTRING_LEN] = {
"Write buffer misses",
"Transmit ring full",
"SPI errors",
+ "Write verify errors",
};
#ifdef CONFIG_DEBUG_FS
diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c
index 66b775d..d531050 100644
--- a/drivers/net/ethernet/qualcomm/qca_spi.c
+++ b/drivers/net/ethernet/qualcomm/qca_spi.c
@@ -69,6 +69,12 @@ static int qcaspi_pluggable = QCASPI_PLUGGABLE_MIN;
module_param(qcaspi_pluggable, int, 0);
MODULE_PARM_DESC(qcaspi_pluggable, "Pluggable SPI connection (yes/no).");
+#define QCASPI_WRITE_VERIFY_MIN 0
+#define QCASPI_WRITE_VERIFY_MAX 3
+static int wr_verify = QCASPI_WRITE_VERIFY_MIN;
+module_param(wr_verify, int, 0);
+MODULE_PARM_DESC(wr_verify, "SPI register write verify trails. Use 0-3.");
+
#define QCASPI_TX_TIMEOUT (1 * HZ)
#define QCASPI_QCA7K_REBOOT_TIME_MS 1000
@@ -77,7 +83,7 @@ start_spi_intr_handling(struct qcaspi *qca, u16 *intr_cause)
{
*intr_cause = 0;
- qcaspi_write_register(qca, SPI_REG_INTR_ENABLE, 0);
+ qcaspi_write_register(qca, SPI_REG_INTR_ENABLE, 0, wr_verify);
qcaspi_read_register(qca, SPI_REG_INTR_CAUSE, intr_cause);
netdev_dbg(qca->net_dev, "interrupts: 0x%04x\n", *intr_cause);
}
@@ -90,8 +96,8 @@ end_spi_intr_handling(struct qcaspi *qca, u16 intr_cause)
SPI_INT_RDBUF_ERR |
SPI_INT_WRBUF_ERR);
- qcaspi_write_register(qca, SPI_REG_INTR_CAUSE, intr_cause);
- qcaspi_write_register(qca, SPI_REG_INTR_ENABLE, intr_enable);
+ qcaspi_write_register(qca, SPI_REG_INTR_CAUSE, intr_cause, 0);
+ qcaspi_write_register(qca, SPI_REG_INTR_ENABLE, intr_enable, wr_verify);
netdev_dbg(qca->net_dev, "acking int: 0x%04x\n", intr_cause);
}
@@ -239,7 +245,7 @@ qcaspi_tx_frame(struct qcaspi *qca, struct sk_buff *skb)
len = skb->len;
- qcaspi_write_register(qca, SPI_REG_BFR_SIZE, len);
+ qcaspi_write_register(qca, SPI_REG_BFR_SIZE, len, wr_verify);
if (qca->legacy_mode)
qcaspi_tx_cmd(qca, QCA7K_SPI_WRITE | QCA7K_SPI_EXTERNAL);
@@ -345,6 +351,7 @@ qcaspi_receive(struct qcaspi *qca)
/* Read the packet size. */
qcaspi_read_register(qca, SPI_REG_RDBUF_BYTE_AVA, &available);
+
netdev_dbg(net_dev, "qcaspi_receive: SPI_REG_RDBUF_BYTE_AVA: Value: %08x\n",
available);
@@ -353,7 +360,7 @@ qcaspi_receive(struct qcaspi *qca)
return -1;
}
- qcaspi_write_register(qca, SPI_REG_BFR_SIZE, available);
+ qcaspi_write_register(qca, SPI_REG_BFR_SIZE, available, wr_verify);
if (qca->legacy_mode)
qcaspi_tx_cmd(qca, QCA7K_SPI_READ | QCA7K_SPI_EXTERNAL);
@@ -524,7 +531,7 @@ qcaspi_qca7k_sync(struct qcaspi *qca, int event)
netdev_dbg(qca->net_dev, "sync: resetting device.\n");
qcaspi_read_register(qca, SPI_REG_SPI_CONFIG, &spi_config);
spi_config |= QCASPI_SLAVE_RESET_BIT;
- qcaspi_write_register(qca, SPI_REG_SPI_CONFIG, spi_config);
+ qcaspi_write_register(qca, SPI_REG_SPI_CONFIG, spi_config, 0);
qca->sync = QCASPI_SYNC_RESET;
qca->stats.trig_reset++;
@@ -684,7 +691,7 @@ qcaspi_netdev_close(struct net_device *dev)
netif_stop_queue(dev);
- qcaspi_write_register(qca, SPI_REG_INTR_ENABLE, 0);
+ qcaspi_write_register(qca, SPI_REG_INTR_ENABLE, 0, wr_verify);
free_irq(qca->spi_dev->irq, qca);
kthread_stop(qca->spi_thread);
@@ -904,6 +911,13 @@ qca_spi_probe(struct spi_device *spi)
return -EINVAL;
}
+ if (wr_verify < QCASPI_WRITE_VERIFY_MIN ||
+ wr_verify > QCASPI_WRITE_VERIFY_MAX) {
+ dev_err(&spi->dev, "Invalid write verify: %d\n",
+ wr_verify);
+ return -EINVAL;
+ }
+
dev_info(&spi->dev, "ver=%s, clkspeed=%d, burst_len=%d, pluggable=%d\n",
QCASPI_DRV_VERSION,
qcaspi_clkspeed,
diff --git a/drivers/net/ethernet/qualcomm/qca_spi.h b/drivers/net/ethernet/qualcomm/qca_spi.h
index fc0e987..2d2c497 100644
--- a/drivers/net/ethernet/qualcomm/qca_spi.h
+++ b/drivers/net/ethernet/qualcomm/qca_spi.h
@@ -73,6 +73,7 @@ struct qcaspi_stats {
u64 write_buf_miss;
u64 ring_full;
u64 spi_err;
+ u64 write_verify_failed;
};
struct qcaspi {
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] wimax/i2400m: fix spelling mistake "unitialized" -> "uninitialized"
From: Sergei Shtylyov @ 2018-09-24 17:27 UTC (permalink / raw)
To: Colin King, Inaky Perez-Gonzalez, linux-wimax, David S . Miller,
netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20180924171853.3294-1-colin.king@canonical.com>
On 09/24/2018 08:18 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Trivial fix to spelling mistake in ms_to_errno array of error messages
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/wimax/i2400m/control.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wimax/i2400m/control.c b/drivers/net/wimax/i2400m/control.c
> index 094cea775d0c..0475ad553707 100644
> --- a/drivers/net/wimax/i2400m/control.c
> +++ b/drivers/net/wimax/i2400m/control.c
> @@ -257,7 +257,7 @@ static const struct
> [I2400M_MS_ACCESSIBILITY_ERROR] = { "accesibility error", -EIO },
> [I2400M_MS_BUSY] = { "busy", -EBUSY },
> [I2400M_MS_CORRUPTED_TLV] = { "corrupted TLV", -EILSEQ },
> - [I2400M_MS_UNINITIALIZED] = { "not unitialized", -EILSEQ },
> + [I2400M_MS_UNINITIALIZED] = { "not uninitialized", -EILSEQ },
I suspect they rather meant "not initialized".
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] wimax/i2400m: fix spelling mistake "unitialized" -> "uninitialized"
From: Colin Ian King @ 2018-09-24 17:35 UTC (permalink / raw)
To: Sergei Shtylyov, Inaky Perez-Gonzalez, linux-wimax,
David S . Miller, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <db8575a7-a4ab-3234-3e59-06cdeffe0f59@cogentembedded.com>
On 24/09/18 18:27, Sergei Shtylyov wrote:
> On 09/24/2018 08:18 PM, Colin King wrote:
>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Trivial fix to spelling mistake in ms_to_errno array of error messages
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>> drivers/net/wimax/i2400m/control.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wimax/i2400m/control.c b/drivers/net/wimax/i2400m/control.c
>> index 094cea775d0c..0475ad553707 100644
>> --- a/drivers/net/wimax/i2400m/control.c
>> +++ b/drivers/net/wimax/i2400m/control.c
>> @@ -257,7 +257,7 @@ static const struct
>> [I2400M_MS_ACCESSIBILITY_ERROR] = { "accesibility error", -EIO },
>> [I2400M_MS_BUSY] = { "busy", -EBUSY },
>> [I2400M_MS_CORRUPTED_TLV] = { "corrupted TLV", -EILSEQ },
>> - [I2400M_MS_UNINITIALIZED] = { "not unitialized", -EILSEQ },
>> + [I2400M_MS_UNINITIALIZED] = { "not uninitialized", -EILSEQ },
>
> I suspect they rather meant "not initialized".
Of course, that makes far more sense. I send a V2.
>
> [...]
>
> MBR, Sergei
>
^ permalink raw reply
* [PATCH][V2] wimax/i2400m: fix spelling mistake "not unitialized" -> "uninitialized"
From: Colin King @ 2018-09-24 17:39 UTC (permalink / raw)
To: Inaky Perez-Gonzalez, linux-wimax, David S . Miller, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Trivial fix to spelling mistake in ms_to_errno array of error messages
and remove confusing "not" from the error text since the error code
refers to an uninitialized error code.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
V2: remove "not" from text too. Thanks to Sergei Shtylyov for spotting that.
---
drivers/net/wimax/i2400m/control.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wimax/i2400m/control.c b/drivers/net/wimax/i2400m/control.c
index 094cea775d0c..0475ad553707 100644
--- a/drivers/net/wimax/i2400m/control.c
+++ b/drivers/net/wimax/i2400m/control.c
@@ -257,7 +257,7 @@ static const struct
[I2400M_MS_ACCESSIBILITY_ERROR] = { "accesibility error", -EIO },
[I2400M_MS_BUSY] = { "busy", -EBUSY },
[I2400M_MS_CORRUPTED_TLV] = { "corrupted TLV", -EILSEQ },
- [I2400M_MS_UNINITIALIZED] = { "not unitialized", -EILSEQ },
+ [I2400M_MS_UNINITIALIZED] = { "uninitialized", -EILSEQ },
[I2400M_MS_UNKNOWN_ERROR] = { "unknown error", -EIO },
[I2400M_MS_PRODUCTION_ERROR] = { "production error", -EIO },
[I2400M_MS_NO_RF] = { "no RF", -EIO },
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 0/2] gpiolib: Fix issues introduced by fast bitmap processing path
From: Marek Szyprowski @ 2018-09-24 11:38 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Linus Walleij, Jonathan Corbet, Miguel Ojeda Sandonis,
Peter Korsgaard, Peter Rosin, Ulf Hansson, Andrew Lunn,
Florian Fainelli, David S. Miller, Dominik Brodowski,
Greg Kroah-Hartman, Kishon Vijay Abraham I, Lars-Peter Clausen,
Michael Hennerich, Jonathan Cameron, Hartmut Knaack,
Peter Meerwald-Stadler, Jiri Slaby, Willy Tarreau
In-Reply-To: <CAGfqbt6H8u_dUokB-pg51kCj09G8Mh7wB0879nKMw5Dw83tcSg@mail.gmail.com>
Hi Janusz,
On 2018-09-24 13:08, Janusz Krzysztofik wrote:
> 2018-09-24 11:43 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>:
>> On 2018-09-24 01:53, Janusz Krzysztofik wrote:
>>> While investigating possible reasons of GPIO fast bitmap processing
>>> related boot hang on Samsung Snow Chromebook, reported by Marek
>>> Szyprowski (thanks!), I've discovered one coding bug, addressed by
>>> PATCH 1/2 of this series, and one potential regression introduced at
>>> design level of the solution, hopefully fixed by PATCH 2/2. See
>>> commit messages for details.
>>>
>>> Janusz Krzysztofik (2):
>>> gpiolib: Fix missing updates of bitmap index
>>> gpiolib: Fix array members of same chip processed separately
>>>
>>> The fixes should resolve the boot hang observed by Marek, however the
>>> second change excludes that particular case from fast bitmap processing
>>> and restores the old behaviour.
>> I confirm, that the above 2 patches fixes boot issue on Samsung Snow
>> Chromebook with next-20180920.
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>>> Hence, it is possible still another
>>> issue which have had an influence on that boot hang exists in the code.
>>> In order to fully verify the fix, it would have to be tested on a
>>> platform where an array of GPIO descriptors is used which starts from
>>> at least two consecutive pins of one GPIO chip in hardware order,
>>> starting ftom 0, followed by one or more pins belonging to other
>>> chip(s).
>>>
>>> In order to verify if separate calls to .set() chip callback for each
>>> pin instead of one call to .set_multiple() is actually the reason of
>>> boot hang on Samsung Snow Chromebook, the affected driver -
>>> drivers/mmc/core/pwrseq_simple.c - would have to be temporarily
>>> modified for testing purposes so it calls gpiod_set_value() for each
>>> pin instead of gpiod_set_array_value() for all of them. If that would
>>> also result in boot hang, we could be sure the issue was really the
>>> one addressed by the second fix. Marek, could you please try to
>>> perform such test?
>> Yes, I've just tested next-20180920 only with the first patch from this
>> patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c.
>> It boots fine, so indeed the issue is in handling of arrays of gpios.
>>
>> Just to be sure I did it right, this is my change to the mentioned file:
> Yeah, that's what I had on mind. However, I'd be more lucky if it didn't work
> for you. Setting the pins sequentially, not simultaneously as before, was
> exactly what I hoped was the reason of the hang.
>
>> diff --git a/drivers/mmc/core/pwrseq_simple.c
>> b/drivers/mmc/core/pwrseq_simple.c
>> index 7f882a2bb872..9397dc1f2e38 100644
>> --- a/drivers/mmc/core/pwrseq_simple.c
>> +++ b/drivers/mmc/core/pwrseq_simple.c
>> @@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct
>> mmc_pwrseq_simple *pwrseq,
>> int value)
>> {
>> struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
>> + int i;
>>
>> - if (!IS_ERR(reset_gpios)) {
>> - DECLARE_BITMAP(values, BITS_PER_TYPE(value));
>> - int nvalues = reset_gpios->ndescs;
>> -
>> - values[0] = value;
>> -
>> - gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
>> - reset_gpios->info, values);
>> - }
>> + if (!IS_ERR(reset_gpios))
>> + for (i = 0; i < reset_gpios->ndescs; i++)
> The only difference from the behaviour when the hang was occurring is now
> the order the pins are manipulated. Maybe that matters?
> Could you please retry the same with the order of pins reversed, either in
> the .dts file or here inside this for loop?
I've switched the order of pins in dts and next-20180920 + first patch +
above
change also boots fine.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* r8169 hang on 4.18
From: Ortwin Glück @ 2018-09-24 12:00 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, netdev
Hi,
Stable kernel has stability problems on r8169 that were not present in 4.17.3:
[ 0.000000] Linux version 4.18.8 (kbuild@lofw) (gcc version 7.3.0 (Gentoo 7.3.0-r3 p1.4)) #70 SMP
PREEMPT Mon Sep 17 17:56:57 CEST 2018
[ 0.000000] Command line: BOOT_IMAGE=/boot/linux-4.18.8 root=LABEL=ROOT ro rootfstype=ext4
net.ifnames=0 pci=nomsi
[ 1.772849] r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
[ 1.772852] r8169 0000:07:00.0: can't disable ASPM; OS doesn't have ASPM control
[ 1.784948] r8169 0000:07:00.0 eth4: RTL8168h/8111h, 50:9a:4c:2e:92:be, XID 54100800, IRQ 16
[ 1.784949] r8169 0000:07:00.0 eth4: jumbo features [frames: 9200 bytes, tx checksumming: ko]
We saw the interface unresponsive twice during the last 3 days with:
[Mon Sep 24 11:35:56 2018] ------------[ cut here ]------------
[Mon Sep 24 11:35:56 2018] NETDEV WATCHDOG: wan (r8169): transmit queue 0 timed out
[Mon Sep 24 11:35:56 2018] WARNING: CPU: 2 PID: 0 at net/sched/sch_generic.c:461
dev_watchdog+0x215/0x220
[Mon Sep 24 11:35:56 2018] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 4.18.8 #70
[Mon Sep 24 11:35:56 2018] Hardware name: Dell Inc. OptiPlex 3050/0W0CHX, BIOS 1.6.5 09/09/2017
[Mon Sep 24 11:35:56 2018] RIP: 0010:dev_watchdog+0x215/0x220
[Mon Sep 24 11:35:56 2018] Code: 49 63 4c 24 e8 eb 8c 4c 89 ef c6 05 1a 19 ca 00 01 e8 5f 52 fd ff
89 d9 4c 89 ee 48 c7 c7 78 ab 67 89 48 89 c2 e8 1b 2b 49 ff <0f> 0b eb be 0f 1f 80 00 00 00 00 41 57
45 89 cf 41 56 49 89 d6 41
[Mon Sep 24 11:35:56 2018] RSP: 0018:ffff96f05dd03e98 EFLAGS: 00010282
[Mon Sep 24 11:35:56 2018] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000006
[Mon Sep 24 11:35:56 2018] RDX: 0000000000000007 RSI: 0000000000000096 RDI: ffff96f05dd15350
[Mon Sep 24 11:35:56 2018] RBP: ffff96f0462ee41c R08: 0000000000000001 R09: 000000000000133d
[Mon Sep 24 11:35:56 2018] R10: 0000000000000202 R11: 0000000000000000 R12: ffff96f0462ee438
[Mon Sep 24 11:35:56 2018] R13: ffff96f0462ee000 R14: 0000000000000001 R15: ffff96f0455eaa80
[Mon Sep 24 11:35:56 2018] FS: 0000000000000000(0000) GS:ffff96f05dd00000(0000) knlGS:0000000000000000
[Mon Sep 24 11:35:56 2018] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[Mon Sep 24 11:35:56 2018] CR2: 000055c9498766e0 CR3: 00000000bb80a006 CR4: 00000000003606e0
[Mon Sep 24 11:35:56 2018] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[Mon Sep 24 11:35:56 2018] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[Mon Sep 24 11:35:56 2018] Call Trace:
[Mon Sep 24 11:35:56 2018] <IRQ>
[Mon Sep 24 11:35:56 2018] ? pfifo_fast_reset+0x130/0x130
[Mon Sep 24 11:35:56 2018] ? pfifo_fast_reset+0x130/0x130
[Mon Sep 24 11:35:56 2018] call_timer_fn+0x11/0x70
[Mon Sep 24 11:35:56 2018] expire_timers+0x8e/0xa0
[Mon Sep 24 11:35:56 2018] run_timer_softirq+0xb9/0x160
[Mon Sep 24 11:35:56 2018] ? __hrtimer_run_queues+0x135/0x1a0
[Mon Sep 24 11:35:56 2018] ? hw_breakpoint_pmu_read+0x10/0x10
[Mon Sep 24 11:35:56 2018] ? ktime_get+0x39/0x90
[Mon Sep 24 11:35:56 2018] ? lapic_next_event+0x20/0x20
[Mon Sep 24 11:35:56 2018] __do_softirq+0xcb/0x1f8
[Mon Sep 24 11:35:56 2018] irq_exit+0xa9/0xb0
[Mon Sep 24 11:35:56 2018] smp_apic_timer_interrupt+0x59/0x90
[Mon Sep 24 11:35:56 2018] apic_timer_interrupt+0xf/0x20
[Mon Sep 24 11:35:56 2018] </IRQ>
[Mon Sep 24 11:35:56 2018] RIP: 0010:cpuidle_enter_state+0x129/0x200
[Mon Sep 24 11:35:56 2018] Code: 45 00 89 c3 e8 d8 3b 55 ff 65 8b 3d b1 eb 45 77 e8 8c 3a 55 ff 31
ff 49 89 c4 e8 72 43 55 ff fb 48 ba cf f7 53 e3 a5 9b c4 20 <4c> 89 e1 4c 29 e9 48 89 c8 48 c1 f9 3f
48 f7 ea b8 ff ff ff 7f 48
[Mon Sep 24 11:35:56 2018] RSP: 0018:ffff9a93c06e7ea8 EFLAGS: 00000286 ORIG_RAX: ffffffffffffff13
[Mon Sep 24 11:35:56 2018] RAX: ffff96f05dd1f800 RBX: 0000000000000003 RCX: 000000000000001f
[Mon Sep 24 11:35:56 2018] RDX: 20c49ba5e353f7cf RSI: 00000000258f0602 RDI: 0000000000000000
[Mon Sep 24 11:35:56 2018] RBP: ffff96f05dd25ee0 R08: 00000000000002b4 R09: 00000000ffffffff
[Mon Sep 24 11:35:56 2018] R10: ffff9a93c06e7e90 R11: 0000000000000142 R12: 00012ec849a182b9
[Mon Sep 24 11:35:56 2018] R13: 00012ec8499ddf88 R14: 0000000000000003 R15: 0000000000000000
[Mon Sep 24 11:35:56 2018] ? cpuidle_enter_state+0x11e/0x200
[Mon Sep 24 11:35:56 2018] do_idle+0x1c0/0x200
[Mon Sep 24 11:35:56 2018] cpu_startup_entry+0x6a/0x70
[Mon Sep 24 11:35:56 2018] start_secondary+0x18a/0x1c0
[Mon Sep 24 11:35:56 2018] secondary_startup_64+0xa5/0xb0
[Mon Sep 24 11:35:56 2018] ---[ end trace 327bd9c035abe307 ]---
This is the built-in ethernet port on a Dell main board:
07:00.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express
Gigabit Ethernet Controller [10ec:8168] (rev 15)
Subsystem: Dell RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller [1028:07a3]
Flags: bus master, fast devsel, latency 0, IRQ 16
I/O ports at e000 [size=256]
Memory at f7404000 (64-bit, non-prefetchable) [size=4K]
Memory at f7400000 (64-bit, non-prefetchable) [size=16K]
Capabilities: [40] Power Management version 3
Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+
Capabilities: [70] Express Endpoint, MSI 01
Capabilities: [b0] MSI-X: Enable- Count=4 Masked-
Capabilities: [100] Advanced Error Reporting
Capabilities: [140] Virtual Channel
Capabilities: [160] Device Serial Number 01-00-00-00-68-4c-e0-00
Capabilities: [170] Latency Tolerance Reporting
Capabilities: [178] L1 PM Substates
Kernel driver in use: r8169
The box has an extra 4-way ethernet card that uses the same driver. We had to set pci=nomsi because
the card frequently behaved erratic with msi on.
Thanks,
Ortwin
^ permalink raw reply
* vlan missing with AF_PACKET and auxdata
From: Michael Walle @ 2018-09-24 12:01 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Eric Dumazet, Steinar H. Gunderson, heiko.thiery
Hi,
I'm using the AF_PACKET socket with setsockopt(PACKET_AUXDATA) to get
the incoming VLAN tag. Correct me if I'm wrong, but as far as I see the
first VLAN tag is always stripped - either in hardware or in
net/core/dev.c in __netif_receive_skb_core() - and stored in
skb->vlan_tci. Therefore, it won't be in the packet data anymore.
If I use the socket with ETH_P_ALL as protocol, everything works as
expected. But if I'm using an actual protocol number, instead of the
catch all, the tp_vlan_tci field inside the auxdata will be zero. I've
traced this to commit d4b812dea4a236f729526facf97df1a9d18e191c (vlan:
mask vlan prio bits):
+ if (unlikely(vlan_tx_tag_present(skb))) {
+ if (vlan_tx_tag_get_id(skb))
+ skb->pkt_type = PACKET_OTHERHOST;
+ /* Note: we might in the future use prio bits
+ * and set skb->priority like in vlan_do_receive()
+ * For the time being, just ignore Priority Code Point
+ */
+ skb->vlan_tci = 0;
+ }
The ptype_all callbacks (which are working as expected) are before this
code and the ptype_specific callbacks (which are not working) are after
this piece of code. I don't know it this is a bug or not, I guess it is
one, at least from the AF_PACKET socket point of view. If not, how I'm
supposed to get the original VLAN tag with socket(AF_PACKET,
my_protocol)?
I also don't understand the commit (message) as the subject suggesets
only the prio bits should be masked, but with "skb->vlan_tci = 0"
everything is masked. Therefore, I've put the original (hopefully the
mail addresses are still valid) authors in CC.
Thanks,
-michael
^ permalink raw reply
* Re: [PATCHv2 bpf-next 07/11] bpf: Add helper to retrieve socket in BPF
From: Daniel Borkmann @ 2018-09-24 12:12 UTC (permalink / raw)
To: Joe Stringer, ast
Cc: netdev, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <20180921171043.20823-8-joe@wand.net.nz>
Hi Joe,
couple of comments inline:
On 09/21/2018 07:10 PM, Joe Stringer wrote:
> This patch adds new BPF helper functions, bpf_sk_lookup_tcp() and
> bpf_sk_lookup_udp() which allows BPF programs to find out if there is a
> socket listening on this host, and returns a socket pointer which the
> BPF program can then access to determine, for instance, whether to
> forward or drop traffic. bpf_sk_lookup_xxx() may take a reference on the
> socket, so when a BPF program makes use of this function, it must
> subsequently pass the returned pointer into the newly added sk_release()
> to return the reference.
>
> By way of example, the following pseudocode would filter inbound
> connections at XDP if there is no corresponding service listening for
> the traffic:
>
> struct bpf_sock_tuple tuple;
> struct bpf_sock_ops *sk;
>
> populate_tuple(ctx, &tuple); // Extract the 5tuple from the packet
> sk = bpf_sk_lookup_tcp(ctx, &tuple, sizeof tuple, netns, 0);
> if (!sk) {
> // Couldn't find a socket listening for this traffic. Drop.
> return TC_ACT_SHOT;
> }
> bpf_sk_release(sk, 0);
> return TC_ACT_OK;
>
> Signed-off-by: Joe Stringer <joe@wand.net.nz>
>
> ---
>
> v2: Rework 'struct bpf_sock_tuple' to allow passing a packet pointer
> Limit netns_id field to 32 bits
> Fix compile error with CONFIG_IPV6 enabled
> Allow direct packet access from helper
> ---
> include/uapi/linux/bpf.h | 57 ++++++++-
> kernel/bpf/verifier.c | 8 +-
> net/core/filter.c | 149 ++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 57 ++++++++-
> tools/testing/selftests/bpf/bpf_helpers.h | 12 ++
> 5 files changed, 280 insertions(+), 3 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index aa5ccd2385ed..620adbb09a94 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2143,6 +2143,41 @@ union bpf_attr {
> * request in the skb.
> * Return
> * 0 on success, or a negative error in case of failure.
> + *
> + * struct bpf_sock_ops *bpf_sk_lookup_tcp(ctx, tuple, tuple_size, netns, flags)
Nit: could you add proper signature like in other cases that are documented?
> + * Decription
> + * Look for TCP socket matching 'tuple'. The return value must
> + * be checked, and if non-NULL, released via bpf_sk_release().
> + * @ctx: pointer to ctx
> + * @tuple: pointer to struct bpf_sock_tuple
> + * @tuple_size: size of the tuple
> + * @netns: network namespace id
> + * @flags: flags value
Should probably say in all three cases that it's unused right now and reserved
for future.
> + * Return
> + * pointer to socket ops on success, or
> + * NULL in case of failure
> + *
> + * struct bpf_sock_ops *bpf_sk_lookup_udp(ctx, tuple, tuple_size, netns, flags)
> + * Decription
> + * Look for UDP socket matching 'tuple'. The return value must
> + * be checked, and if non-NULL, released via bpf_sk_release().
> + * @ctx: pointer to ctx
> + * @tuple: pointer to struct bpf_sock_tuple
> + * @tuple_size: size of the tuple
> + * @netns: network namespace id
> + * @flags: flags value
> + * Return
> + * pointer to socket ops on success, or
> + * NULL in case of failure
> + *
> + * int bpf_sk_release(sock, flags)
> + * Description
> + * Release the reference held by 'sock'.
> + * @sock: Pointer reference to release. Must be found via
> + * bpf_sk_lookup_xxx().
> + * @flags: flags value
> + * Return
> + * 0 on success, or a negative error in case of failure.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2228,7 +2263,10 @@ union bpf_attr {
> FN(get_current_cgroup_id), \
> FN(get_local_storage), \
> FN(sk_select_reuseport), \
> - FN(skb_ancestor_cgroup_id),
> + FN(skb_ancestor_cgroup_id), \
> + FN(sk_lookup_tcp), \
> + FN(sk_lookup_udp), \
> + FN(sk_release),
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
> @@ -2398,6 +2436,23 @@ struct bpf_sock {
> */
> };
>
> +struct bpf_sock_tuple {
> + union {
> + struct {
> + __be32 saddr;
> + __be32 daddr;
> + __be16 sport;
> + __be16 dport;
> + } ipv4;
> + struct {
> + __be32 saddr[4];
> + __be32 daddr[4];
> + __be16 sport;
> + __be16 dport;
> + } ipv6;
> + };
> +};
> +
> #define XDP_PACKET_HEADROOM 256
>
> /* User return codes for XDP prog type.
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 16818508b225..7b7fa94aba58 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -153,6 +153,12 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
> * PTR_TO_MAP_VALUE, PTR_TO_SOCKET_OR_NULL becomes PTR_TO_SOCKET when the type
> * passes through a NULL-check conditional. For the branch wherein the state is
> * changed to CONST_IMM, the verifier releases the reference.
> + *
> + * For each helper function that allocates a reference, such as
> + * bpf_sk_lookup_tcp(), there is a corresponding release function, such as
> + * bpf_sk_release(). When a reference type passes into the release function,
> + * the verifier also releases the reference. If any unchecked or unreleased
> + * reference remains at the end of the program, the verifier rejects it.
> */
>
> /* verifier_state + insn_idx are pushed to stack when branch is encountered */
> @@ -300,7 +306,7 @@ static bool arg_type_is_refcounted(enum bpf_arg_type type)
> */
> static bool is_release_function(enum bpf_func_id func_id)
> {
> - return false;
> + return func_id == BPF_FUNC_sk_release;
> }
>
> /* string representation of 'enum bpf_reg_type' */
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 43f81883f31d..f0715e2e7b07 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -58,13 +58,17 @@
> #include <net/busy_poll.h>
> #include <net/tcp.h>
> #include <net/xfrm.h>
> +#include <net/udp.h>
> #include <linux/bpf_trace.h>
> #include <net/xdp_sock.h>
> #include <linux/inetdevice.h>
> +#include <net/inet_hashtables.h>
> +#include <net/inet6_hashtables.h>
> #include <net/ip_fib.h>
> #include <net/flow.h>
> #include <net/arp.h>
> #include <net/ipv6.h>
> +#include <net/net_namespace.h>
> #include <linux/seg6_local.h>
> #include <net/seg6.h>
> #include <net/seg6_local.h>
> @@ -4812,6 +4816,139 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
> };
> #endif /* CONFIG_IPV6_SEG6_BPF */
>
> +struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
> + struct sk_buff *skb, u8 family, u8 proto)
> +{
> + int dif = skb->dev->ifindex;
> + bool refcounted = false;
> + struct sock *sk = NULL;
> +
> + if (family == AF_INET) {
> + __be32 src4 = tuple->ipv4.saddr;
> + __be32 dst4 = tuple->ipv4.daddr;
> + int sdif = inet_sdif(skb);
> +
> + if (proto == IPPROTO_TCP)
> + sk = __inet_lookup(net, &tcp_hashinfo, skb, 0,
> + src4, tuple->ipv4.sport,
> + dst4, tuple->ipv4.dport,
> + dif, sdif, &refcounted);
> + else
> + sk = __udp4_lib_lookup(net, src4, tuple->ipv4.sport,
> + dst4, tuple->ipv4.dport,
> + dif, sdif, &udp_table, skb);
> +#if IS_ENABLED(CONFIG_IPV6)
> + } else {
> + struct in6_addr *src6 = (struct in6_addr *)&tuple->ipv6.saddr;
> + struct in6_addr *dst6 = (struct in6_addr *)&tuple->ipv6.daddr;
> + int sdif = inet6_sdif(skb);
> +
> + if (proto == IPPROTO_TCP)
> + sk = __inet6_lookup(net, &tcp_hashinfo, skb, 0,
> + src6, tuple->ipv6.sport,
> + dst6, tuple->ipv6.dport,
> + dif, sdif, &refcounted);
> + else
> + sk = __udp6_lib_lookup(net, src6, tuple->ipv6.sport,
> + dst6, tuple->ipv6.dport,
> + dif, sdif, &udp_table, skb);
> +#endif
> + }
> +
> + if (unlikely(sk && !refcounted && !sock_flag(sk, SOCK_RCU_FREE))) {
> + WARN_ONCE(1, "Found non-RCU, unreferenced socket!");
> + sk = NULL;
> + }
> + return sk;
> +}
> +
> +/* bpf_sk_lookup performs the core lookup for different types of sockets,
> + * taking a reference on the socket if it doesn't have the flag SOCK_RCU_FREE.
> + * Returns the socket as an 'unsigned long' to simplify the casting in the
> + * callers to satisfy BPF_CALL declarations.
> + */
> +static unsigned long
> +bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> + u8 proto, u64 netns_id, u64 flags)
> +{
> + struct net *caller_net = dev_net(skb->dev);
For sk_skb programs, are we *always* guaranteed to have a skb->dev assigned?
This definitely holds true for tc programs, but afaik not for sk_skb ones where
you enable the helpers below, so this would result in a NULL ptr dereference.
> + struct sock *sk = NULL;
> + u8 family = AF_UNSPEC;
> + struct net *net;
> +
> + family = len == sizeof(tuple->ipv4) ? AF_INET : AF_INET6;
> + if (unlikely(family == AF_UNSPEC || netns_id > U32_MAX || flags))
> + goto out;
> +
> + if (netns_id)
> + net = get_net_ns_by_id(caller_net, netns_id);
> + else
> + net = caller_net;
> + if (unlikely(!net))
> + goto out;
> + sk = sk_lookup(net, tuple, skb, family, proto);
> + put_net(net);
> +
> + if (sk)
> + sk = sk_to_full_sk(sk);
> +out:
> + return (unsigned long) sk;
> +}
> +
> +BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
> + struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
> +{
> + return bpf_sk_lookup(skb, tuple, len, IPPROTO_TCP, netns_id, flags);
> +}
> +
> +static const struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
> + .func = bpf_sk_lookup_tcp,
> + .gpl_only = false,
> + .pkt_access = true,
> + .ret_type = RET_PTR_TO_SOCKET_OR_NULL,
> + .arg1_type = ARG_PTR_TO_CTX,
> + .arg2_type = ARG_PTR_TO_MEM,
> + .arg3_type = ARG_CONST_SIZE,
> + .arg4_type = ARG_ANYTHING,
> + .arg5_type = ARG_ANYTHING,
> +};
> +
> +BPF_CALL_5(bpf_sk_lookup_udp, struct sk_buff *, skb,
> + struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
> +{
> + return bpf_sk_lookup(skb, tuple, len, IPPROTO_UDP, netns_id, flags);
> +}
> +
> +static const struct bpf_func_proto bpf_sk_lookup_udp_proto = {
> + .func = bpf_sk_lookup_udp,
> + .gpl_only = false,
> + .pkt_access = true,
> + .ret_type = RET_PTR_TO_SOCKET_OR_NULL,
> + .arg1_type = ARG_PTR_TO_CTX,
> + .arg2_type = ARG_PTR_TO_MEM,
> + .arg3_type = ARG_CONST_SIZE,
> + .arg4_type = ARG_ANYTHING,
> + .arg5_type = ARG_ANYTHING,
> +};
> +
> +BPF_CALL_2(bpf_sk_release, struct sock *, sk, u64, flags)
> +{
> + if (!sock_flag(sk, SOCK_RCU_FREE))
> + sock_gen_put(sk);
> +
> + if (unlikely(flags))
> + return -EINVAL;
I guess it's probably okay to leave here, though I'm wondering whether it's
worth to have a flags part in general in this helper. We need to release the
reference in any case beforehands anyway as we otherwise leak it. Personally,
I'd probably that arg here.
> + return 0;
> +}
> +
> +static const struct bpf_func_proto bpf_sk_release_proto = {
> + .func = bpf_sk_release,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_SOCKET,
> + .arg2_type = ARG_ANYTHING,
> +};
> +
> bool bpf_helper_changes_pkt_data(void *func)
> {
> if (func == bpf_skb_vlan_push ||
> @@ -5018,6 +5155,12 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> case BPF_FUNC_skb_ancestor_cgroup_id:
> return &bpf_skb_ancestor_cgroup_id_proto;
> #endif
> + case BPF_FUNC_sk_lookup_tcp:
> + return &bpf_sk_lookup_tcp_proto;
> + case BPF_FUNC_sk_lookup_udp:
> + return &bpf_sk_lookup_udp_proto;
> + case BPF_FUNC_sk_release:
> + return &bpf_sk_release_proto;
> default:
> return bpf_base_func_proto(func_id);
> }
> @@ -5118,6 +5261,12 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_sk_redirect_hash_proto;
> case BPF_FUNC_get_local_storage:
> return &bpf_get_local_storage_proto;
> + case BPF_FUNC_sk_lookup_tcp:
> + return &bpf_sk_lookup_tcp_proto;
> + case BPF_FUNC_sk_lookup_udp:
> + return &bpf_sk_lookup_udp_proto;
> + case BPF_FUNC_sk_release:
> + return &bpf_sk_release_proto;
(See comment above.)
> default:
> return bpf_base_func_proto(func_id);
> }
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index aa5ccd2385ed..620adbb09a94 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -2143,6 +2143,41 @@ union bpf_attr {
> * request in the skb.
> * Return
> * 0 on success, or a negative error in case of failure.
> + *
> + * struct bpf_sock_ops *bpf_sk_lookup_tcp(ctx, tuple, tuple_size, netns, flags)
> + * Decription
> + * Look for TCP socket matching 'tuple'. The return value must
> + * be checked, and if non-NULL, released via bpf_sk_release().
> + * @ctx: pointer to ctx
> + * @tuple: pointer to struct bpf_sock_tuple
> + * @tuple_size: size of the tuple
> + * @netns: network namespace id
> + * @flags: flags value
> + * Return
> + * pointer to socket ops on success, or
> + * NULL in case of failure
> + *
> + * struct bpf_sock_ops *bpf_sk_lookup_udp(ctx, tuple, tuple_size, netns, flags)
> + * Decription
> + * Look for UDP socket matching 'tuple'. The return value must
> + * be checked, and if non-NULL, released via bpf_sk_release().
> + * @ctx: pointer to ctx
> + * @tuple: pointer to struct bpf_sock_tuple
> + * @tuple_size: size of the tuple
> + * @netns: network namespace id
> + * @flags: flags value
> + * Return
> + * pointer to socket ops on success, or
> + * NULL in case of failure
> + *
> + * int bpf_sk_release(sock, flags)
> + * Description
> + * Release the reference held by 'sock'.
> + * @sock: Pointer reference to release. Must be found via
> + * bpf_sk_lookup_xxx().
> + * @flags: flags value
> + * Return
> + * 0 on success, or a negative error in case of failure.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2228,7 +2263,10 @@ union bpf_attr {
> FN(get_current_cgroup_id), \
> FN(get_local_storage), \
> FN(sk_select_reuseport), \
> - FN(skb_ancestor_cgroup_id),
> + FN(skb_ancestor_cgroup_id), \
> + FN(sk_lookup_tcp), \
> + FN(sk_lookup_udp), \
> + FN(sk_release),
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
> @@ -2398,6 +2436,23 @@ struct bpf_sock {
> */
> };
>
> +struct bpf_sock_tuple {
> + union {
> + struct {
> + __be32 saddr;
> + __be32 daddr;
> + __be16 sport;
> + __be16 dport;
> + } ipv4;
> + struct {
> + __be32 saddr[4];
> + __be32 daddr[4];
> + __be16 sport;
> + __be16 dport;
> + } ipv6;
> + };
> +};
> +
> #define XDP_PACKET_HEADROOM 256
>
> /* User return codes for XDP prog type.
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index e4be7730222d..88ce00c3aa0f 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -143,6 +143,18 @@ static unsigned long long (*bpf_skb_cgroup_id)(void *ctx) =
> (void *) BPF_FUNC_skb_cgroup_id;
> static unsigned long long (*bpf_skb_ancestor_cgroup_id)(void *ctx, int level) =
> (void *) BPF_FUNC_skb_ancestor_cgroup_id;
> +static struct bpf_sock *(*bpf_sk_lookup_tcp)(void *ctx,
> + struct bpf_sock_tuple *tuple,
> + int size, unsigned int netns_id,
> + unsigned long long flags) =
> + (void *) BPF_FUNC_sk_lookup_tcp;
> +static struct bpf_sock *(*bpf_sk_lookup_udp)(void *ctx,
> + struct bpf_sock_tuple *tuple,
> + int size, unsigned int netns_id,
> + unsigned long long flags) =
> + (void *) BPF_FUNC_sk_lookup_udp;
> +static int (*bpf_sk_release)(struct bpf_sock *sk, unsigned long long flags) =
> + (void *) BPF_FUNC_sk_release;
>
> /* llvm builtin functions that eBPF C program may use to
> * emit BPF_LD_ABS and BPF_LD_IND instructions
>
^ permalink raw reply
* Re: [PATCH] wireless: ipw2x00: Remove unnecessary parentheses
From: Stanislav Yakovlev @ 2018-09-24 18:21 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: Kalle Valo, linux-wireless, netdev, linux-kernel
In-Reply-To: <20180920204548.2405-1-natechancellor@gmail.com>
On 21/09/2018, Nathan Chancellor <natechancellor@gmail.com> wrote:
> Clang warns when multiple pairs of parentheses are used for a single
> conditional statement.
>
> drivers/net/wireless/intel/ipw2x00/ipw2200.c:5655:28: warning: equality
> comparison with extraneous parentheses [-Wparentheses-equality]
> if ((priv->ieee->iw_mode == IW_MODE_ADHOC)) {
> ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
> drivers/net/wireless/intel/ipw2x00/ipw2200.c:5655:28: note: remove
> extraneous parentheses around the comparison to silence this warning
> if ((priv->ieee->iw_mode == IW_MODE_ADHOC)) {
> ~ ^ ~
> drivers/net/wireless/intel/ipw2x00/ipw2200.c:5655:28: note: use '=' to
> turn this equality comparison into an assignment
> if ((priv->ieee->iw_mode == IW_MODE_ADHOC)) {
> ^~
> =
> 1 warning generated.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/134
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> drivers/net/wireless/intel/ipw2x00/ipw2200.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Looks fine, thanks!
Stanislav.
^ permalink raw reply
* Re: [PATCHv2 bpf-next 08/11] selftests/bpf: Add tests for reference tracking
From: Daniel Borkmann @ 2018-09-24 12:21 UTC (permalink / raw)
To: Joe Stringer, ast
Cc: netdev, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <20180921171043.20823-9-joe@wand.net.nz>
On 09/21/2018 07:10 PM, Joe Stringer wrote:
> reference tracking: leak potential reference
> reference tracking: leak potential reference on stack
> reference tracking: leak potential reference on stack 2
> reference tracking: zero potential reference
> reference tracking: copy and zero potential references
> reference tracking: release reference without check
> reference tracking: release reference
> reference tracking: release reference twice
> reference tracking: release reference twice inside branch
> reference tracking: alloc, check, free in one subbranch
> reference tracking: alloc, check, free in both subbranches
> reference tracking in call: free reference in subprog
> reference tracking in call: free reference in subprog and outside
> reference tracking in call: alloc & leak reference in subprog
> reference tracking in call: alloc in subprog, release outside
> reference tracking in call: sk_ptr leak into caller stack
> reference tracking in call: sk_ptr spill into caller stack
>
> Signed-off-by: Joe Stringer <joe@wand.net.nz>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 359 ++++++++++++++++++++
> 1 file changed, 359 insertions(+)
I think this here needs to have some more test cases that we current do not track but
should in order to have better coverage. At minimum what comes to mind additionally:
- verifier interaction with LD_ABS, LD_IND
- verifier interaction with tail calls (e.g. try to leak socket, socket_or_null, etc,
but should also have a positive test where we drop ref before tail call to show it
works in combination)
- Try to mangle a socket and socket_or_null pointer with ALU ops and pass it to helper
- Try to access the socket data fields after we released its reference
- Access socket member fields in general (I think not present right now)
- Use direct packet access in combination with lookup helper (it's enabled
via pkt_access = true in the helper, so we should also test for it here to make
sure future changes don't break it)
- ...
^ permalink raw reply
* Re: vlan missing with AF_PACKET and auxdata
From: Jan Grashöfer @ 2018-09-24 12:22 UTC (permalink / raw)
To: Michael Walle, netdev
Cc: David S. Miller, Eric Dumazet, Steinar H. Gunderson, heiko.thiery
In-Reply-To: <9a284a4d29bb3301f50d7993c6bebb04@walle.cc>
Hi Michael,
On 24/09/2018 14:01, Michael Walle wrote:
> I'm using the AF_PACKET socket with setsockopt(PACKET_AUXDATA) to get
> the incoming VLAN tag. Correct me if I'm wrong, but as far as I see the
> first VLAN tag is always stripped - either in hardware or in
> net/core/dev.c in __netif_receive_skb_core() - and stored in
> skb->vlan_tci. Therefore, it won't be in the packet data anymore.
although the documentation says "SOCK_RAW packets are passed to and from
the device driver without any changes in the packet data." [1] that's
correct and was discussed here:
https://www.spinics.net/lists/netdev/msg440313.html
Result of the discussion: Won't fix, too complicated.
Jan
[1] http://man7.org/linux/man-pages/man7/packet.7.html
^ permalink raw reply
* Re: [PATCHv2 bpf-next 04/11] bpf: Add PTR_TO_SOCKET verifier type
From: Daniel Borkmann @ 2018-09-24 12:24 UTC (permalink / raw)
To: Joe Stringer, ast
Cc: netdev, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <20180921171043.20823-5-joe@wand.net.nz>
On 09/21/2018 07:10 PM, Joe Stringer wrote:
> Teach the verifier a little bit about a new type of pointer, a
> PTR_TO_SOCKET. This pointer type is accessed from BPF through the
> 'struct bpf_sock' structure.
>
> Signed-off-by: Joe Stringer <joe@wand.net.nz>
>
> ---
>
> v2: Reuse reg_type_mismatch() in more places
> Reduce the number of passes at convert_ctx_access()
> ---
> include/linux/bpf.h | 17 +++++
> include/linux/bpf_verifier.h | 2 +
> kernel/bpf/verifier.c | 120 +++++++++++++++++++++++++++++++----
> net/core/filter.c | 30 +++++----
> 4 files changed, 143 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 988a00797bcd..daeb0d343d9c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -154,6 +154,7 @@ enum bpf_arg_type {
>
> ARG_PTR_TO_CTX, /* pointer to context */
> ARG_ANYTHING, /* any (initialized) argument is ok */
> + ARG_PTR_TO_SOCKET, /* pointer to bpf_sock */
> };
>
> /* type of values returned from helper functions */
> @@ -162,6 +163,7 @@ enum bpf_return_type {
> RET_VOID, /* function doesn't return anything */
> RET_PTR_TO_MAP_VALUE, /* returns a pointer to map elem value */
> RET_PTR_TO_MAP_VALUE_OR_NULL, /* returns a pointer to map elem value or NULL */
> + RET_PTR_TO_SOCKET_OR_NULL, /* returns a pointer to a socket or NULL */
> };
>
> /* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs
> @@ -213,6 +215,8 @@ enum bpf_reg_type {
> PTR_TO_PACKET, /* reg points to skb->data */
> PTR_TO_PACKET_END, /* skb->data + headlen */
> PTR_TO_FLOW_KEYS, /* reg points to bpf_flow_keys */
> + PTR_TO_SOCKET, /* reg points to struct bpf_sock */
> + PTR_TO_SOCKET_OR_NULL, /* reg points to struct bpf_sock or NULL */
> };
>
> /* The information passed from prog-specific *_is_valid_access
> @@ -335,6 +339,11 @@ const struct bpf_func_proto *bpf_get_trace_printk_proto(void);
>
> typedef unsigned long (*bpf_ctx_copy_t)(void *dst, const void *src,
> unsigned long off, unsigned long len);
> +typedef u32 (*bpf_convert_ctx_access_t)(enum bpf_access_type type,
> + const struct bpf_insn *src,
> + struct bpf_insn *dst,
> + struct bpf_prog *prog,
> + u32 *target_size);
>
> u64 bpf_event_output(struct bpf_map *map, u64 flags, void *meta, u64 meta_size,
> void *ctx, u64 ctx_size, bpf_ctx_copy_t ctx_copy);
> @@ -828,4 +837,12 @@ extern const struct bpf_func_proto bpf_get_local_storage_proto;
> void bpf_user_rnd_init_once(void);
> u64 bpf_user_rnd_u32(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
>
> +bool bpf_sock_is_valid_access(int off, int size, enum bpf_access_type type,
> + struct bpf_insn_access_aux *info);
> +u32 bpf_sock_convert_ctx_access(enum bpf_access_type type,
> + const struct bpf_insn *si,
> + struct bpf_insn *insn_buf,
> + struct bpf_prog *prog,
> + u32 *target_size);
> +
> #endif /* _LINUX_BPF_H */
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index af262b97f586..23a2b17bfd75 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -58,6 +58,8 @@ struct bpf_reg_state {
> * offset, so they can share range knowledge.
> * For PTR_TO_MAP_VALUE_OR_NULL this is used to share which map value we
> * came from, when one is tested for != NULL.
> + * For PTR_TO_SOCKET this is used to share which pointers retain the
> + * same reference to the socket, to determine proper reference freeing.
> */
> u32 id;
> /* For scalar types (SCALAR_VALUE), this represents our knowledge of
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7dccb18ede03..1fee63d82290 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -80,8 +80,8 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
> * (like pointer plus pointer becomes SCALAR_VALUE type)
> *
> * When verifier sees load or store instructions the type of base register
> - * can be: PTR_TO_MAP_VALUE, PTR_TO_CTX, PTR_TO_STACK. These are three pointer
> - * types recognized by check_mem_access() function.
> + * can be: PTR_TO_MAP_VALUE, PTR_TO_CTX, PTR_TO_STACK, PTR_TO_SOCKET. These are
> + * four pointer types recognized by check_mem_access() function.
> *
> * PTR_TO_MAP_VALUE means that this register is pointing to 'map element value'
> * and the range of [ptr, ptr + map's value_size) is accessible.
> @@ -267,6 +267,8 @@ static const char * const reg_type_str[] = {
> [PTR_TO_PACKET_META] = "pkt_meta",
> [PTR_TO_PACKET_END] = "pkt_end",
> [PTR_TO_FLOW_KEYS] = "flow_keys",
> + [PTR_TO_SOCKET] = "sock",
> + [PTR_TO_SOCKET_OR_NULL] = "sock_or_null",
> };
>
> static char slot_type_char[] = {
> @@ -973,6 +975,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
> case PTR_TO_PACKET_END:
> case PTR_TO_FLOW_KEYS:
> case CONST_PTR_TO_MAP:
> + case PTR_TO_SOCKET:
> + case PTR_TO_SOCKET_OR_NULL:
> return true;
> default:
> return false;
> @@ -1341,6 +1345,28 @@ static int check_flow_keys_access(struct bpf_verifier_env *env, int off,
> return 0;
> }
>
> +static int check_sock_access(struct bpf_verifier_env *env, u32 regno, int off,
> + int size, enum bpf_access_type t)
> +{
> + struct bpf_reg_state *regs = cur_regs(env);
> + struct bpf_reg_state *reg = ®s[regno];
> + struct bpf_insn_access_aux info;
> +
> + if (reg->smin_value < 0) {
> + verbose(env, "R%d min value is negative, either use unsigned index or do a if (index >=0) check.\n",
> + regno);
> + return -EACCES;
> + }
> +
> + if (!bpf_sock_is_valid_access(off, size, t, &info)) {
> + verbose(env, "invalid bpf_sock_ops access off=%d size=%d\n",
> + off, size);
> + return -EACCES;
> + }
> +
> + return 0;
> +}
> +
> static bool __is_pointer_value(bool allow_ptr_leaks,
> const struct bpf_reg_state *reg)
> {
> @@ -1459,6 +1485,9 @@ static int check_ptr_alignment(struct bpf_verifier_env *env,
> */
> strict = true;
> break;
> + case PTR_TO_SOCKET:
> + pointer_desc = "sock ";
> + break;
> default:
> break;
> }
> @@ -1726,6 +1755,14 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
> err = check_flow_keys_access(env, off, size);
> if (!err && t == BPF_READ && value_regno >= 0)
> mark_reg_unknown(env, regs, value_regno);
> + } else if (reg->type == PTR_TO_SOCKET) {
> + if (t == BPF_WRITE) {
> + verbose(env, "cannot write into socket\n");
> + return -EACCES;
> + }
> + err = check_sock_access(env, regno, off, size, t);
> + if (!err && value_regno >= 0)
> + mark_reg_unknown(env, regs, value_regno);
> } else {
> verbose(env, "R%d invalid mem access '%s'\n", regno,
> reg_type_str[reg->type]);
> @@ -1949,6 +1986,10 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
> err = check_ctx_reg(env, reg, regno);
> if (err < 0)
> return err;
> + } else if (arg_type == ARG_PTR_TO_SOCKET) {
> + expected_type = PTR_TO_SOCKET;
> + if (type != expected_type)
> + goto err_type;
> } else if (arg_type_is_mem_ptr(arg_type)) {
> expected_type = PTR_TO_STACK;
> /* One exception here. In case function allows for NULL to be
> @@ -2542,6 +2583,10 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> }
> regs[BPF_REG_0].map_ptr = meta.map_ptr;
> regs[BPF_REG_0].id = ++env->id_gen;
> + } else if (fn->ret_type == RET_PTR_TO_SOCKET_OR_NULL) {
> + mark_reg_known_zero(env, regs, BPF_REG_0);
> + regs[BPF_REG_0].type = PTR_TO_SOCKET_OR_NULL;
> + regs[BPF_REG_0].id = ++env->id_gen;
> } else {
> verbose(env, "unknown return type %d of func %s#%d\n",
> fn->ret_type, func_id_name(func_id), func_id);
> @@ -2679,6 +2724,8 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> return -EACCES;
> case CONST_PTR_TO_MAP:
> case PTR_TO_PACKET_END:
> + case PTR_TO_SOCKET:
> + case PTR_TO_SOCKET_OR_NULL:
> verbose(env, "R%d pointer arithmetic on %s prohibited\n",
> dst, reg_type_str[ptr_reg->type]);
> return -EACCES;
> @@ -3626,6 +3673,8 @@ static void mark_ptr_or_null_reg(struct bpf_reg_state *reg, u32 id,
> } else {
> reg->type = PTR_TO_MAP_VALUE;
> }
> + } else if (reg->type == PTR_TO_SOCKET_OR_NULL) {
> + reg->type = PTR_TO_SOCKET;
> }
> /* We don't need id from this point onwards anymore, thus we
> * should better reset it, so that state pruning has chances
> @@ -4401,6 +4450,8 @@ static bool regsafe(struct bpf_reg_state *rold, struct bpf_reg_state *rcur,
> case CONST_PTR_TO_MAP:
> case PTR_TO_PACKET_END:
> case PTR_TO_FLOW_KEYS:
> + case PTR_TO_SOCKET:
> + case PTR_TO_SOCKET_OR_NULL:
> /* Only valid matches are exact, which memcmp() above
> * would have accepted
> */
> @@ -4678,6 +4729,37 @@ static int is_state_visited(struct bpf_verifier_env *env, int insn_idx)
> return 0;
> }
>
> +/* Return true if it's OK to have the same insn return a different type. */
> +static bool reg_type_mismatch_ok(enum bpf_reg_type type)
> +{
> + switch (type) {
> + case PTR_TO_CTX:
> + case PTR_TO_SOCKET:
> + case PTR_TO_SOCKET_OR_NULL:
> + return false;
> + default:
> + return true;
> + }
> +}
> +
> +/* If an instruction was previously used with particular pointer types, then we
> + * need to be careful to avoid cases such as the below, where it may be ok
> + * for one branch accessing the pointer, but not ok for the other branch:
> + *
> + * R1 = sock_ptr
> + * goto X;
> + * ...
> + * R1 = some_other_valid_ptr;
> + * goto X;
> + * ...
> + * R2 = *(u32 *)(R1 + 0);
> + */
> +static bool reg_type_mismatch(enum bpf_reg_type src, enum bpf_reg_type prev)
> +{
> + return src != prev && (!reg_type_mismatch_ok(src) ||
> + !reg_type_mismatch_ok(prev));
> +}
> +
> static int do_check(struct bpf_verifier_env *env)
> {
> struct bpf_verifier_state *state;
> @@ -4810,9 +4892,7 @@ static int do_check(struct bpf_verifier_env *env)
> */
> *prev_src_type = src_reg_type;
>
> - } else if (src_reg_type != *prev_src_type &&
> - (src_reg_type == PTR_TO_CTX ||
> - *prev_src_type == PTR_TO_CTX)) {
> + } else if (reg_type_mismatch(src_reg_type, *prev_src_type)) {
> /* ABuser program is trying to use the same insn
> * dst_reg = *(u32*) (src_reg + off)
> * with different pointer types:
> @@ -4857,9 +4937,7 @@ static int do_check(struct bpf_verifier_env *env)
>
> if (*prev_dst_type == NOT_INIT) {
> *prev_dst_type = dst_reg_type;
> - } else if (dst_reg_type != *prev_dst_type &&
> - (dst_reg_type == PTR_TO_CTX ||
> - *prev_dst_type == PTR_TO_CTX)) {
> + } else if (reg_type_mismatch(dst_reg_type, *prev_dst_type)) {
> verbose(env, "same insn cannot be used with different pointers\n");
> return -EINVAL;
> }
> @@ -5276,8 +5354,10 @@ static void sanitize_dead_code(struct bpf_verifier_env *env)
> }
> }
>
> -/* convert load instructions that access fields of 'struct __sk_buff'
> - * into sequence of instructions that access fields of 'struct sk_buff'
> +/* convert load instructions that access fields of a context type into a
> + * sequence of instructions that access fields of the underlying structure:
> + * struct __sk_buff -> struct sk_buff
> + * struct bpf_sock_ops -> struct sock
> */
> static int convert_ctx_accesses(struct bpf_verifier_env *env)
> {
> @@ -5306,12 +5386,14 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> }
> }
>
> - if (!ops->convert_ctx_access || bpf_prog_is_dev_bound(env->prog->aux))
> + if (bpf_prog_is_dev_bound(env->prog->aux))
> return 0;
>
> insn = env->prog->insnsi + delta;
>
> for (i = 0; i < insn_cnt; i++, insn++) {
> + bpf_convert_ctx_access_t convert_ctx_access;
> +
> if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) ||
> insn->code == (BPF_LDX | BPF_MEM | BPF_H) ||
> insn->code == (BPF_LDX | BPF_MEM | BPF_W) ||
> @@ -5353,8 +5435,18 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> continue;
> }
>
> - if (env->insn_aux_data[i + delta].ptr_type != PTR_TO_CTX)
> + switch (env->insn_aux_data[i + delta].ptr_type) {
> + case PTR_TO_CTX:
> + if (!ops->convert_ctx_access)
> + continue;
> + convert_ctx_access = ops->convert_ctx_access;
> + break;
> + case PTR_TO_SOCKET:
> + convert_ctx_access = bpf_sock_convert_ctx_access;
> + break;
> + default:
> continue;
> + }
>
> ctx_field_size = env->insn_aux_data[i + delta].ctx_field_size;
> size = BPF_LDST_BYTES(insn);
When you compile without NET, this would need to have a stub implementation as
otherwise it might break build. We recently tripped over it in bpf flow dissector
as well.
^ permalink raw reply
* Re: [PATCHv2 bpf-next 07/11] bpf: Add helper to retrieve socket in BPF
From: Daniel Borkmann @ 2018-09-24 12:38 UTC (permalink / raw)
To: Joe Stringer, ast
Cc: netdev, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <d2b803a4-509f-6d7f-1177-f3ccc1f875eb@iogearbox.net>
On 09/24/2018 02:12 PM, Daniel Borkmann wrote:
> Hi Joe,
>
> couple of comments inline:
>
> On 09/21/2018 07:10 PM, Joe Stringer wrote:
>> This patch adds new BPF helper functions, bpf_sk_lookup_tcp() and
>> bpf_sk_lookup_udp() which allows BPF programs to find out if there is a
>> socket listening on this host, and returns a socket pointer which the
>> BPF program can then access to determine, for instance, whether to
>> forward or drop traffic. bpf_sk_lookup_xxx() may take a reference on the
>> socket, so when a BPF program makes use of this function, it must
>> subsequently pass the returned pointer into the newly added sk_release()
>> to return the reference.
>>
>> By way of example, the following pseudocode would filter inbound
>> connections at XDP if there is no corresponding service listening for
>> the traffic:
>>
>> struct bpf_sock_tuple tuple;
>> struct bpf_sock_ops *sk;
>>
>> populate_tuple(ctx, &tuple); // Extract the 5tuple from the packet
>> sk = bpf_sk_lookup_tcp(ctx, &tuple, sizeof tuple, netns, 0);
>> if (!sk) {
>> // Couldn't find a socket listening for this traffic. Drop.
>> return TC_ACT_SHOT;
>> }
>> bpf_sk_release(sk, 0);
>> return TC_ACT_OK;
>>
>> Signed-off-by: Joe Stringer <joe@wand.net.nz>
>>
>> ---
>>
>> v2: Rework 'struct bpf_sock_tuple' to allow passing a packet pointer
>> Limit netns_id field to 32 bits
>> Fix compile error with CONFIG_IPV6 enabled
>> Allow direct packet access from helper
>> ---
>> include/uapi/linux/bpf.h | 57 ++++++++-
>> kernel/bpf/verifier.c | 8 +-
>> net/core/filter.c | 149 ++++++++++++++++++++++
>> tools/include/uapi/linux/bpf.h | 57 ++++++++-
>> tools/testing/selftests/bpf/bpf_helpers.h | 12 ++
>> 5 files changed, 280 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index aa5ccd2385ed..620adbb09a94 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -2143,6 +2143,41 @@ union bpf_attr {
>> * request in the skb.
>> * Return
>> * 0 on success, or a negative error in case of failure.
>> + *
>> + * struct bpf_sock_ops *bpf_sk_lookup_tcp(ctx, tuple, tuple_size, netns, flags)
>
> Nit: could you add proper signature like in other cases that are documented?
>
>> + * Decription
>> + * Look for TCP socket matching 'tuple'. The return value must
>> + * be checked, and if non-NULL, released via bpf_sk_release().
>> + * @ctx: pointer to ctx
>> + * @tuple: pointer to struct bpf_sock_tuple
>> + * @tuple_size: size of the tuple
>> + * @netns: network namespace id
>> + * @flags: flags value
>
> Should probably say in all three cases that it's unused right now and reserved
> for future.
I think the two lookup helpers here also need to have a better documentation in
terms of semantics wrt netns, so it definitely needs to be documented when it's
derived from the skb's dev and when netns is used.
>> + * Return
>> + * pointer to socket ops on success, or
>> + * NULL in case of failure
>> + *
>> + * struct bpf_sock_ops *bpf_sk_lookup_udp(ctx, tuple, tuple_size, netns, flags)
>> + * Decription
>> + * Look for UDP socket matching 'tuple'. The return value must
>> + * be checked, and if non-NULL, released via bpf_sk_release().
>> + * @ctx: pointer to ctx
>> + * @tuple: pointer to struct bpf_sock_tuple
>> + * @tuple_size: size of the tuple
>> + * @netns: network namespace id
>> + * @flags: flags value
>> + * Return
>> + * pointer to socket ops on success, or
>> + * NULL in case of failure
>> + *
>> + * int bpf_sk_release(sock, flags)
>> + * Description
>> + * Release the reference held by 'sock'.
>> + * @sock: Pointer reference to release. Must be found via
>> + * bpf_sk_lookup_xxx().
>> + * @flags: flags value
>> + * Return
>> + * 0 on success, or a negative error in case of failure.
>> */
^ permalink raw reply
* OFFICE
From: OFFICE @ 2018-09-24 12:40 UTC (permalink / raw)
[-- Attachment #1: Type: text/plain, Size: 1 bytes --]
[-- Attachment #2: OFFICE.png --]
[-- Type: image/png, Size: 39871 bytes --]
^ permalink raw reply
* Re: [PATCHv2 bpf-next 07/11] bpf: Add helper to retrieve socket in BPF
From: Daniel Borkmann @ 2018-09-24 12:51 UTC (permalink / raw)
To: Joe Stringer, ast
Cc: netdev, john.fastabend, tgraf, kafai, nitin.hande,
mauricio.vasquez
In-Reply-To: <d2b803a4-509f-6d7f-1177-f3ccc1f875eb@iogearbox.net>
On 09/24/2018 02:12 PM, Daniel Borkmann wrote:
> On 09/21/2018 07:10 PM, Joe Stringer wrote:
[...]
>> +/* bpf_sk_lookup performs the core lookup for different types of sockets,
>> + * taking a reference on the socket if it doesn't have the flag SOCK_RCU_FREE.
>> + * Returns the socket as an 'unsigned long' to simplify the casting in the
>> + * callers to satisfy BPF_CALL declarations.
>> + */
>> +static unsigned long
>> +bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
>> + u8 proto, u64 netns_id, u64 flags)
>> +{
>> + struct net *caller_net = dev_net(skb->dev);
>
> For sk_skb programs, are we *always* guaranteed to have a skb->dev assigned?
>
> This definitely holds true for tc programs, but afaik not for sk_skb ones where
> you enable the helpers below, so this would result in a NULL ptr dereference.
>
>> + struct sock *sk = NULL;
>> + u8 family = AF_UNSPEC;
>> + struct net *net;
>> +
>> + family = len == sizeof(tuple->ipv4) ? AF_INET : AF_INET6;
>> + if (unlikely(family == AF_UNSPEC || netns_id > U32_MAX || flags))
>> + goto out;
>> +
>> + if (netns_id)
>> + net = get_net_ns_by_id(caller_net, netns_id);
>> + else
>> + net = caller_net;
>> + if (unlikely(!net))
>> + goto out;
>> + sk = sk_lookup(net, tuple, skb, family, proto);
>> + put_net(net);
Hmm, isn't this also resulting in a use-after-free on net ?
Presume we use dev_net(skb->dev) as net. It does read_pnet(&dev->nd_net), which
either gets you pnet->net when netns is configured or &init_net otherwise.
I don't see where an additional reference was taken in this path, hence you'll
get an imbalance which triggers freeing the underlying netns ..
>> + if (sk)
>> + sk = sk_to_full_sk(sk);
>> +out:
>> + return (unsigned long) sk;
>> +}
>> +
>> +BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
>> + struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
>> +{
>> + return bpf_sk_lookup(skb, tuple, len, IPPROTO_TCP, netns_id, flags);
>> +}
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox