* Re: [PATCHv6 07/10] acpi/hmat: Register processor domain to its memory
From: Rafael J. Wysocki @ 2019-02-24 19:59 UTC (permalink / raw)
To: Keith Busch
Cc: Rafael J. Wysocki, Linux Kernel Mailing List,
ACPI Devel Maling List, Linux Memory Management List, Linux API,
Greg Kroah-Hartman, Dave Hansen, Dan Williams
In-Reply-To: <20190222184831.GF10237@localhost.localdomain>
On Fri, Feb 22, 2019 at 7:48 PM Keith Busch <keith.busch@intel.com> wrote:
>
> On Wed, Feb 20, 2019 at 11:02:01PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Feb 14, 2019 at 6:10 PM Keith Busch <keith.busch@intel.com> wrote:
> > > config ACPI_HMAT
> > > bool "ACPI Heterogeneous Memory Attribute Table Support"
> > > depends on ACPI_NUMA
> > > + select HMEM_REPORTING
> >
> > If you want to do this here, I'm not sure that defining HMEM_REPORTING
> > as a user-selectable option is a good idea. In particular, I don't
> > really think that setting ACPI_HMAT without it makes a lot of sense.
> > Apart from this, the patch looks reasonable to me.
>
> I'm trying to implement based on the feedback, but I'm a little confused.
>
> As I have it at the moment, HMEM_REPORTING is not user-prompted, so
> another option needs to turn it on. I have ACPI_HMAT do that here.
>
> So when you say it's a bad idea to make HMEM_REPORTING user selectable,
> isn't it already not user selectable?
I thought that HMEM_REPORTING was user-prompted initially, by bad if it wasn't.
> If I do it the other way around, that's going to make HMEM_REPORTING
> complicated if a non-ACPI implementation wants to report HMEM
> properties.
But the mitigations that Dave was talking about get in the way, don't they?
Say there is another Kconfig option,CACHE_MITIGATIONS, to enable them.
Then you want ACPI_HMAT to be set when that it set and you also want
ACPI_HMAT to be set when HMEM_REPORTING and ACPI_NUMA are both set.
OTOH, you may not want HMEM_REPORTING to be set when CACHE_MITIGATIONS
is set, but that causes ACPI_HMAT to be set and which means that
ACPI_HMAT alone will not be sufficient to determine the
HMEM_REPORTING value.
Now, if you prompt for HMEM_REPORTING and make it depend on ACPI_NUMA,
then ACPI_HMAT can be selected by that (regardless of the
CACHE_MITIGATIONS value).
And if someone wants to use HMEM_REPORTING without ACPI_NUMA, it can
be made depend on whatever new option is there for that non-ACPI
mechanism.
There might be a problem if someone wanted to enable the alternative
way of HMEM_REPORTING if ACPI_NUMA was set (in which case HMAT would
have to be ignored even if it was present), but in that case there
would need to be an explicit way to choose between HMAT and non-HMAT
anyway.
In any case, I prefer providers to be selected by consumers and not
the other way around, in case there are multiple consumers for one
provider.
^ permalink raw reply
* Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: kbuild test robot @ 2019-02-24 11:37 UTC (permalink / raw)
Cc: kbuild-all, ast, daniel, netdev, linux-api, Lorenz Bauer
In-Reply-To: <20190222095057.9442-2-lmb@cloudflare.com>
[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]
Hi Lorenz,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
[also build test ERROR on next-20190222]
[cannot apply to v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Lorenz-Bauer/Allow-checking-SYN-cookies-from-XDP-and-tc-cls-act/20190224-180755
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: m68k-sun3_defconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=m68k
All errors (new ones prefixed by >>):
m68k-linux-gnu-ld: drivers/rtc/proc.o: in function `is_rtc_hctosys.isra.0':
proc.c:(.text+0x178): undefined reference to `strcmp'
m68k-linux-gnu-ld: net/core/filter.o: in function `bpf_sk_check_syncookie':
>> filter.c:(.text+0x5a58): undefined reference to `__cookie_v6_check'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12145 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: kbuild test robot @ 2019-02-24 11:21 UTC (permalink / raw)
Cc: kbuild-all, ast, daniel, netdev, linux-api, Lorenz Bauer
In-Reply-To: <20190222095057.9442-2-lmb@cloudflare.com>
[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]
Hi Lorenz,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
[also build test ERROR on next-20190222]
[cannot apply to v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Lorenz-Bauer/Allow-checking-SYN-cookies-from-XDP-and-tc-cls-act/20190224-180755
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-kexec (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
net/core/filter.c: In function '____bpf_sk_check_syncookie':
>> net/core/filter.c:5477:10: error: 'ENOTSUP' undeclared (first use in this function); did you mean 'ENOTSUPP'?
return -ENOTSUP;
^~~~~~~
ENOTSUPP
net/core/filter.c:5477:10: note: each undeclared identifier is reported only once for each function it appears in
>> net/core/filter.c:5479:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
vim +5477 net/core/filter.c
5467
5468 default:
5469 return -EPROTONOSUPPORT;
5470 }
5471
5472 if (ret > 0)
5473 return 0;
5474
5475 return -ENOENT;
5476 #else
> 5477 return -ENOTSUP;
5478 #endif
> 5479 }
5480
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26394 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 0/6] Allow setting file birth time with utimensat()
From: Andreas Dilger @ 2019-02-23 18:32 UTC (permalink / raw)
To: Omar Sandoval
Cc: Dave Chinner, linux-fsdevel, Al Viro, kernel-team, Linux API,
linux-btrfs, Ext4 Developers List, linux-f2fs-devel, linux-xfs,
Theodore Ts'o, Jaegeuk Kim, Steve French
In-Reply-To: <20190222190048.GA29624@vader>
[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]
> On Feb 22, 2019, at 11:00 AM, Omar Sandoval <osandov@osandov.com> wrote:
>
> On Tue, Feb 19, 2019 at 09:18:20AM +1100, Dave Chinner wrote:
>> On Sat, Feb 16, 2019 at 06:57:45PM -0700, Andreas Dilger wrote:
>>> While it may be a bit of a stretch to call this "forensic evidence",
>>
>> We do forensic analysis of corrupt filesystems looking for evidence
>> of what went wrong, not just looking for evidence of what happened
>> on systems that have been broken into.
>>
>>> making it hard to change from except via total root compromise by a
>>> skilled hacker is very useful.
>>
>> *nod*.
>>
>>> If this were to go in (which I'm not in favour of), then there would
>>> need to be a CONFIG and/or runtime knob to turn it off (or better to
>>> only turn it on), similar to how FIPS and other security options can
>>> only go in one direction.
>>
>> The problem here is that "inode birth time" is being conflated with
>> "user document creation time". These two things are very different.
>>
>> i.e. One is filesystem internal information and is not related to
>> when the original copy of the data in the file was created, the
>> other is user specified metadata that is related to the file data
>> contents and needs to travel with the data, not the filesystem.
>>
>> IMO, trying to make one on-disk field hold two different types of
>> information defeats one or the other purpose, and nobody knows which
>> one the field stores for any given file.
>>
>> I'd suggest that "authored date" should be a generic system xattr so
>> most filesystems support it, not just those that have a birth time
>> field on disk. Sure, modify it through utimesat() and expose it
>> through statx() (as authored time, not birth time), but store it a
>> system xattr rather than an internal filesystem metadata field that
>> requires was never intended to be user modifiable.
>
> It seems that this is the general consensus, so I'll look into
> implementing this functionality as an xattr.
I would recommend to look at how Samba is storing these attributes
today, and do the same thing, maybe add support into GNU coreutils
to handle this transparently.
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: Martin Lau @ 2019-02-23 0:44 UTC (permalink / raw)
To: Lorenz Bauer
Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org,
linux-api@vger.kernel.org
In-Reply-To: <20190222095057.9442-2-lmb@cloudflare.com>
On Fri, Feb 22, 2019 at 09:50:55AM +0000, Lorenz Bauer wrote:
> Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
> to a known connection. However, there is one corner case: no sockets are
> created if SYN cookies are active. This means that the final ACK in the
> 3WHS is misclassified.
>
> Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
> and then check whether a packet is a valid SYN cookie ACK.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> include/uapi/linux/bpf.h | 18 ++++++++++-
> net/core/filter.c | 68 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index bcdd2474eee7..bc2af87e9621 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2359,6 +2359,21 @@ union bpf_attr {
> * Return
> * A **struct bpf_tcp_sock** pointer on success, or NULL in
> * case of failure.
> + *
> + * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> + * Description
> + * Check whether iph and th contain a valid SYN cookie ACK for
> + * the listening socket in sk.
> + *
> + * iph points to the start of the IPv4 or IPv6 header, while
> + * iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
> + *
> + * th points to the start of the TCP header, while th_len contains
> + * sizeof(struct tcphdr).
> + *
> + * Return
> + * 0 if iph and th are a valid SYN cookie ACK, or a negative error
> + * otherwise.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2457,7 +2472,8 @@ union bpf_attr {
> FN(spin_lock), \
> FN(spin_unlock), \
> FN(sk_fullsock), \
> - FN(tcp_sock),
> + FN(tcp_sock), \
> + FN(sk_check_syncookie),
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 85749f6ec789..9e68897cc7ed 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
> .arg1_type = ARG_PTR_TO_SOCK_COMMON,
> };
>
> +BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
s/bpf_sk_check_syncookie/bpf_tcp_check_syncookie/
> + struct tcphdr *, th, u32, th_len)
> +{
> +#if IS_ENABLED(CONFIG_SYN_COOKIES)
nit. "#ifdef CONFIG_SYN_COOKIES" such that it is clear it is a bool kconfig.
> + u32 cookie;
> + int ret;
> +
> + if (unlikely(th_len < sizeof(*th)))
> + return -EINVAL;
> +
> + /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> + if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
>From the test program in patch 3, the "sk" here is obtained from
bpf_sk_lookup_tcp() which does a sk_to_full_sk() before returning.
AFAICT, meaning bpf_sk_lookup_tcp() will return the listening sk
even if there is a request_sock. Does it make sense to check
syncookie if there is already a request_sock?
> + return -EINVAL;
> +
> + if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
Should tcp_synq_no_recent_overflow(tp) be checked also?
> + return -EINVAL;
> +
> + if (!th->ack || th->rst)
How about th->syn?
> + return -ENOENT;
> +
> + cookie = ntohl(th->ack_seq) - 1;
> +
> + switch (sk->sk_family) {
> + case AF_INET:
> + if (unlikely(iph_len < sizeof(struct iphdr)))
> + return -EINVAL;
> +
> + ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
> + break;
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> + case AF_INET6:
> + if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> + return -EINVAL;
> +
> + ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
> + break;
> +#endif /* CONFIG_IPV6 */
> +
> + default:
> + return -EPROTONOSUPPORT;
> + }
> +
> + if (ret > 0)
> + return 0;
> +
> + return -ENOENT;
> +#else
> + return -ENOTSUP;
> +#endif
> +}
> +
> +static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
> + .func = bpf_sk_check_syncookie,
> + .gpl_only = true,
> + .pkt_access = true,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_SOCKET,
I think it should be ARG_PTR_TO_TCP_SOCK
> + .arg2_type = ARG_PTR_TO_MEM,
> + .arg3_type = ARG_CONST_SIZE,
> + .arg4_type = ARG_PTR_TO_MEM,
> + .arg5_type = ARG_CONST_SIZE,
> +};
> +
> #endif /* CONFIG_INET */
^ permalink raw reply
* Re: [PATCHSET v15] io_uring IO interface
From: Jens Axboe @ 2019-02-22 22:32 UTC (permalink / raw)
To: Marek Majkowski
Cc: Avi Kivity, hch, Jann Horn, jmoyer, linux-aio, linux-api,
linux-block, viro
In-Reply-To: <CAJPywTLGBRviKYSf+AQg-0eOZ3pnyTFEhr28no9Zrh8ftsSxmQ@mail.gmail.com>
On 2/22/19 8:01 AM, Marek Majkowski wrote:
> On Thu, Feb 21, 2019 at 6:48 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 2/21/19 5:10 AM, Marek Majkowski wrote:
>>>> From: Jens Axboe <axboe@kernel.dk>
>>>> Subject: [PATCHSET v15] io_uring IO interface
>>>> Message-ID: <20190211190049.7888-1-axboe@kernel.dk> (raw)
>>>>
>>>> Some final tweaks, mostly cosmetic, but also two important fixes:
>>>>
>>>> 1) Ensure that we account the skb appropriately against the socket.
>>>> Some network config options apparently return is an skb with
>>>> ->truesize != 0 when allocated with a size of 0, ensure we add
>>>> those as references against sock->sk_wmem_alloc. Reported by
>>>> Matt Mullins.
>>>
>>> Jens,
>>>
>>> I tried using io_uring with network sockets. It seem to be doing the
>>> right thing. One bit is missing though: "flags" as in recv(2).
>>>
>>> In perfect world I would like to specify at least:
>>> - MSG_DONTWAIT
>>> - MSG_WAITALL
>>> - MSG_NOSIGNAL
>>>
>>> Right now, unless I'm missing something, io_uring_sqe doesn't have a
>>> place where we could store these. "flags" is needed for any
>>> non-trivial network I/O.
>>
>> We have flags for sqes, depending on the type. You can add to the
>> union that already holds rw_flags/fsync_flags/poll_events? There's
>> also a (smaller) flags field that applies for all types, which
>> currently only holds the fixed file flag.
>
> The "sqe->flags" right now is used by the IOSQE_FIXED_FILE which has
> the same value as MSG_OOB.
>
> Sticking recv/send flags into the "rw_flags" union perhaps could work,
> barring the discussion about naming. The obvious names don't make
> sense. recv_flags, send_flags or socket_flags don't sound right.
>
> If we tried to add networking stuff to io_uring (for batchinig and async), then:
> - send()/recv() could work, only needs the "flags" field
> - sendmsg()/recvmsg() likewise
> - sendto()/recvfrom() require two more pointers: (struct sockaddr
> *dest_addr, socklen_t addrlen)
> - sendmmsg() / recvmmsg() are perhaps irrelevant
>
> Non-blocking stuff like socket(), setsockopt(), bind() perhaps don't
> need to be considered, although could benefit from batching.
If we just do separate opcodes for them, then there's 32 bits of flag
space for each one. That should be more than adequate.
> Not sure what to think about connect() and accept(). In the
> prehistoric days there seem to have been an attempt to add socket
> things to libaio struct iocb. See:
>
> https://code.woboq.org/linux/include/libaio.h.html#iocb::(anonymous)::saddr
>
> struct iocb {
> ...
> union {
> ...
> struct io_iocb_sockaddr saddr;
> } u;
> };
>
> Are there chances of reserving space for two pointers in io_uring_sqe,
> which could be used for sendto/recvfrom/accept if we decided to add
> more network support?
There is already space for that. We have 3 x 64 bits at the end of the
sqe, 16 of those are used for the fixed buffers which networking
probably wants to support as well. But that still leaves 112 bits of
space for things opcode specific data.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 14/19] io_uring: add file set registration
From: Jens Axboe @ 2019-02-22 22:29 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity,
Al Viro
In-Reply-To: <CAG48ez25XYh1-SRw7HG-RXOz9xVZ0EXCumT_Csw0ER62K4Q1qw@mail.gmail.com>
On 2/19/19 9:12 AM, Jann Horn wrote:
> On Mon, Feb 11, 2019 at 8:01 PM Jens Axboe <axboe@kernel.dk> wrote:
>> We normally have to fget/fput for each IO we do on a file. Even with
>> the batching we do, the cost of the atomic inc/dec of the file usage
>> count adds up.
>>
>> This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
>> for the io_uring_register(2) system call. The arguments passed in must
>> be an array of __s32 holding file descriptors, and nr_args should hold
>> the number of file descriptors the application wishes to pin for the
>> duration of the io_uring instance (or until IORING_UNREGISTER_FILES is
>> called).
>>
>> When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
>> member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
>> to the index in the array passed in to IORING_REGISTER_FILES.
>>
>> Files are automatically unregistered when the io_uring instance is torn
>> down. An application need only unregister if it wishes to register a new
>> set of fds.
>>
>> Reviewed-by: Hannes Reinecke <hare@suse.com>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
> [...]
>> @@ -1335,6 +1379,161 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
>> return READ_ONCE(ring->r.head) == READ_ONCE(ring->r.tail) ? ret : 0;
>> }
>>
>> +static void __io_sqe_files_unregister(struct io_ring_ctx *ctx)
>> +{
>> +#if defined(CONFIG_UNIX)
>> + if (ctx->ring_sock) {
>> + struct sock *sock = ctx->ring_sock->sk;
>> + struct sk_buff *skb;
>> +
>> + while ((skb = skb_dequeue(&sock->sk_receive_queue)) != NULL)
>> + kfree_skb(skb);
>> + }
>> +#else
>> + int i;
>> +
>> + for (i = 0; i < ctx->nr_user_files; i++)
>> + fput(ctx->user_files[i]);
>> +#endif
>> +}
>> +
>> +static int io_sqe_files_unregister(struct io_ring_ctx *ctx)
>> +{
>> + if (!ctx->user_files)
>> + return -ENXIO;
>> +
>> + __io_sqe_files_unregister(ctx);
>> + kfree(ctx->user_files);
>> + ctx->user_files = NULL;
>> + return 0;
>> +}
>> +
>> +#if defined(CONFIG_UNIX)
>> +/*
>> + * Ensure the UNIX gc is aware of our file set, so we are certain that
>> + * the io_uring can be safely unregistered on process exit, even if we have
>> + * loops in the file referencing.
>> + */
>
> I still don't get how this is supposed to work. Quoting from an
> earlier version of the patch:
>
> |> I think the overall concept here is still broken: You're giving the
> |> user_files to the GC, and I think the GC can drop their refcounts, but
> |> I don't see you actually getting feedback from the GC anywhere that
> |> would let the GC break your references? E.g. in io_prep_rw() you grab
> |> file pointers from ctx->user_files after simply checking
> |> ctx->nr_user_files, and there is no path from the GC that touches
> |> those fields. As far as I can tell, the GC is just going to go through
> |> unix_destruct_scm() and drop references on your files, causing
> |> use-after-free.
> |>
> |> But the unix GC is complicated, and maybe I'm just missing something...
> |
> | Only when the skb is released, which is either done when the io_uring
> | is torn down (and then definitely safe), or if the socket is released,
> | which is again also at a safe time.
>
> I'll try to add inline comments on my understanding of the code, maybe
> you can point out where exactly we're understanding it differently...
>
>> +static int __io_sqe_files_scm(struct io_ring_ctx *ctx, int nr, int offset)
>> +{
>> + struct sock *sk = ctx->ring_sock->sk;
>> + struct scm_fp_list *fpl;
>> + struct sk_buff *skb;
>> + int i;
>> +
>> + fpl = kzalloc(sizeof(*fpl), GFP_KERNEL);
>> + if (!fpl)
>> + return -ENOMEM;
>> +
> // here we allocate a new `skb` with ->users==1
>> + skb = alloc_skb(0, GFP_KERNEL);
>> + if (!skb) {
>> + kfree(fpl);
>> + return -ENOMEM;
>> + }
>> +
>> + skb->sk = sk;
> // set the skb's destructor, invoked when ->users drops to 0;
> // destructor drops file refcounts
>> + skb->destructor = unix_destruct_scm;
>> +
>> + fpl->user = get_uid(ctx->user);
>> + for (i = 0; i < nr; i++) {
> // grab a reference to each file for the skb
>> + fpl->fp[i] = get_file(ctx->user_files[i + offset]);
>> + unix_inflight(fpl->user, fpl->fp[i]);
>> + }
>> +
>> + fpl->max = fpl->count = nr;
>> + UNIXCB(skb).fp = fpl;
>> + refcount_add(skb->truesize, &sk->sk_wmem_alloc);
> // put the skb in the sk_receive_queue, still with a refcount of 1.
>> + skb_queue_head(&sk->sk_receive_queue, skb);
>> +
> // drop a reference from each file; after this, only the
> skb owns references to files;
> // the ctx->user_files entries borrow their lifetime from the skb
>> + for (i = 0; i < nr; i++)
>> + fput(fpl->fp[i]);
>> +
>> + return 0;
>> +}
>
> So let's say you have a cyclic dependency where an io_uring points to
> a unix domain socket, and the unix domain socket points back at the
> uring. The last reference from outside the loop goes away when the
> user closes the uring's fd, but the uring's busypolling kernel thread
> is still running and busypolling for new submission queue entries.
>
> The GC can then come along and run scan_inflight(), detect that
> ctx->ring_sock->sk->sk_receive_queue contains a reference to a unix
> domain socket, and steal the skb (unlinking it from the ring_sock and
> linking it into the hitlist):
>
> __skb_unlink(skb, &x->sk_receive_queue);
> __skb_queue_tail(hitlist, skb);
>
> And then the hitlist will be processed by __skb_queue_purge(),
> dropping the refcount of the skb from 1 to 0. At that point, the unix
> domain socket can be freed, and you still have a pointer to it in
> ctx->user_files.
I've fixed this for the sq offload case.
>> +
>> +/*
>> + * If UNIX sockets are enabled, fd passing can cause a reference cycle which
>> + * causes regular reference counting to break down. We rely on the UNIX
>> + * garbage collection to take care of this problem for us.
>> + */
>> +static int io_sqe_files_scm(struct io_ring_ctx *ctx)
>> +{
>> + unsigned left, total;
>> + int ret = 0;
>> +
>> + total = 0;
>> + left = ctx->nr_user_files;
>> + while (left) {
>> + unsigned this_files = min_t(unsigned, left, SCM_MAX_FD);
>> + int ret;
>> +
>> + ret = __io_sqe_files_scm(ctx, this_files, total);
>> + if (ret)
>> + break;
>
> If we bail out in the middle of translating the ->user_files here, we
> have to make sure that we both destroy the already-created SKBs and
> drop our references on the files we haven't dealt with yet.
Good catch, fixed.
>> + left -= this_files;
>> + total += this_files;
>> + }
>> +
>> + return ret;
>> +}
>> +#else
>> +static int io_sqe_files_scm(struct io_ring_ctx *ctx)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
>> + unsigned nr_args)
>> +{
>> + __s32 __user *fds = (__s32 __user *) arg;
>> + int fd, ret = 0;
>> + unsigned i;
>> +
>> + if (ctx->user_files)
>> + return -EBUSY;
>> + if (!nr_args)
>> + return -EINVAL;
>> + if (nr_args > IORING_MAX_FIXED_FILES)
>> + return -EMFILE;
>> +
>> + ctx->user_files = kcalloc(nr_args, sizeof(struct file *), GFP_KERNEL);
>> + if (!ctx->user_files)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < nr_args; i++) {
>> + ret = -EFAULT;
>> + if (copy_from_user(&fd, &fds[i], sizeof(fd)))
>> + break;
>> +
>> + ctx->user_files[i] = fget(fd);
>> +
>> + ret = -EBADF;
>> + if (!ctx->user_files[i])
>> + break;
>
> Let's say we hit this error condition after N successful loop
> iterations, on a kernel with CONFIG_UNIX. At that point, we've filled
> N file pointers into ctx->user_files[], and we've incremented
> ctx->nr_user_files up to N. Now we jump to the `if (ret)` branch,
> which goes into io_sqe_files_unregister(); but that's going to attempt
> to dequeue inflight files from ctx->ring_sock, so that's not going to
> work.
Fixed, thanks.
>> + /*
>> + * Don't allow io_uring instances to be registered. If UNIX
>> + * isn't enabled, then this causes a reference cycle and this
>> + * instance can never get freed. If UNIX is enabled we'll
>> + * handle it just fine, but there's still no point in allowing
>> + * a ring fd as it doesn't support regular read/write anyway.
>> + */
>> + if (ctx->user_files[i]->f_op == &io_uring_fops) {
>> + fput(ctx->user_files[i]);
>> + break;
>> + }
>> + ctx->nr_user_files++;
>
> I don't see anything that can set ctx->nr_user_files back down to
> zero; as far as I can tell, if you repeatedly register and unregister
> a set of files, ctx->nr_user_files will just grow, and since it's used
> as an upper bound for array accesses, that's bad.
Fixed that one earlier.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 12/19] io_uring: add support for pre-mapped user IO buffers
From: Jens Axboe @ 2019-02-22 22:29 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity,
Al Viro
In-Reply-To: <CAG48ez2jrdFEATYor3-tRUx=LBpXo9581tPCWUgjWf6atMiGzA@mail.gmail.com>
On 2/19/19 12:08 PM, Jann Horn wrote:
> On Mon, Feb 11, 2019 at 8:01 PM Jens Axboe <axboe@kernel.dk> wrote:
>> If we have fixed user buffers, we can map them into the kernel when we
>> setup the io_uring. That avoids the need to do get_user_pages() for
>> each and every IO.
>>
>> To utilize this feature, the application must call io_uring_register()
>> after having setup an io_uring instance, passing in
>> IORING_REGISTER_BUFFERS as the opcode. The argument must be a pointer to
>> an iovec array, and the nr_args should contain how many iovecs the
>> application wishes to map.
>>
>> If successful, these buffers are now mapped into the kernel, eligible
>> for IO. To use these fixed buffers, the application must use the
>> IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED opcodes, and then
>> set sqe->index to the desired buffer index. sqe->addr..sqe->addr+seq->len
>> must point to somewhere inside the indexed buffer.
>>
>> The application may register buffers throughout the lifetime of the
>> io_uring instance. It can call io_uring_register() with
>> IORING_UNREGISTER_BUFFERS as the opcode to unregister the current set of
>> buffers, and then register a new set. The application need not
>> unregister buffers explicitly before shutting down the io_uring
>> instance.
>>
>> It's perfectly valid to setup a larger buffer, and then sometimes only
>> use parts of it for an IO. As long as the range is within the originally
>> mapped region, it will work just fine.
>>
>> For now, buffers must not be file backed. If file backed buffers are
>> passed in, the registration will fail with -1/EOPNOTSUPP. This
>> restriction may be relaxed in the future.
>>
>> RLIMIT_MEMLOCK is used to check how much memory we can pin. A somewhat
>> arbitrary 1G per buffer size is also imposed.
>>
>> Reviewed-by: Hannes Reinecke <hare@suse.com>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
> [...]
>> static void io_sq_wq_submit_work(struct work_struct *work)
>> {
>> struct io_kiocb *req = container_of(work, struct io_kiocb, work);
>> struct sqe_submit *s = &req->submit;
>> const struct io_uring_sqe *sqe = s->sqe;
>> struct io_ring_ctx *ctx = req->ctx;
>> - mm_segment_t old_fs = get_fs();
>> + mm_segment_t old_fs;
>> + bool needs_user;
>> int ret;
>>
>> /* Ensure we clear previously set forced non-block flag */
>> req->flags &= ~REQ_F_FORCE_NONBLOCK;
>> req->rw.ki_flags &= ~IOCB_NOWAIT;
>>
>> - if (!mmget_not_zero(ctx->sqo_mm)) {
>> - ret = -EFAULT;
>> - goto err;
>> - }
>> -
>> - use_mm(ctx->sqo_mm);
>> - set_fs(USER_DS);
>> - s->has_user = true;
>> s->needs_lock = true;
>> + s->has_user = false;
>> +
>> + /*
>> + * If we're doing IO to fixed buffers, we don't need to get/set
>> + * user context
>> + */
>> + needs_user = io_sqe_needs_user(s->sqe);
>> + if (needs_user) {
>> + if (!mmget_not_zero(ctx->sqo_mm)) {
>> + ret = -EFAULT;
>> + goto err;
>> + }
>> + use_mm(ctx->sqo_mm);
>> + old_fs = get_fs();
>> + set_fs(USER_DS);
>> + s->has_user = true;
>> + }
>>
>> do {
>> ret = __io_submit_sqe(ctx, req, s, false, NULL);
>> @@ -1011,9 +1110,11 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>> cond_resched();
>> } while (1);
>>
>> - set_fs(old_fs);
>> - unuse_mm(ctx->sqo_mm);
>> - mmput(ctx->sqo_mm);
>> + if (needs_user) {
>> + set_fs(old_fs);
>> + unuse_mm(ctx->sqo_mm);
>> + mmput(ctx->sqo_mm);
>> + }
>> err:
>> if (ret) {
>> io_cqring_add_event(ctx, sqe->user_data, ret, 0);
>> @@ -1308,6 +1409,197 @@ static unsigned long ring_pages(unsigned sq_entries, unsigned cq_entries)
>> return (bytes + PAGE_SIZE - 1) / PAGE_SIZE;
>> }
>>
>> +static int io_sqe_buffer_unregister(struct io_ring_ctx *ctx)
>> +{
>> + int i, j;
>> +
>> + if (!ctx->user_bufs)
>> + return -ENXIO;
>> +
>> + for (i = 0; i < ctx->nr_user_bufs; i++) {
>> + struct io_mapped_ubuf *imu = &ctx->user_bufs[i];
>> +
>> + for (j = 0; j < imu->nr_bvecs; j++)
>> + put_page(imu->bvec[j].bv_page);
>> +
>> + if (ctx->account_mem)
>> + io_unaccount_mem(ctx->user, imu->nr_bvecs);
>> + kfree(imu->bvec);
>> + imu->nr_bvecs = 0;
>> + }
>> +
>> + kfree(ctx->user_bufs);
>> + ctx->user_bufs = NULL;
>> + ctx->nr_user_bufs = 0;
>> + return 0;
>> +}
> [...]
>> +static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg,
>> + unsigned nr_args)
>> +{
>> + struct vm_area_struct **vmas = NULL;
>> + struct page **pages = NULL;
>> + int i, j, got_pages = 0;
>> + int ret = -EINVAL;
>> +
>> + if (ctx->user_bufs)
>> + return -EBUSY;
>> + if (!nr_args || nr_args > UIO_MAXIOV)
>> + return -EINVAL;
>> +
>> + ctx->user_bufs = kcalloc(nr_args, sizeof(struct io_mapped_ubuf),
>> + GFP_KERNEL);
>> + if (!ctx->user_bufs)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < nr_args; i++) {
>> + struct io_mapped_ubuf *imu = &ctx->user_bufs[i];
>> + unsigned long off, start, end, ubuf;
>> + int pret, nr_pages;
>> + struct iovec iov;
>> + size_t size;
>> +
>> + ret = io_copy_iov(ctx, &iov, arg, i);
>> + if (ret)
>> + break;
>> +
>> + /*
>> + * Don't impose further limits on the size and buffer
>> + * constraints here, we'll -EINVAL later when IO is
>> + * submitted if they are wrong.
>> + */
>> + ret = -EFAULT;
>> + if (!iov.iov_base || !iov.iov_len)
>> + goto err;
>> +
>> + /* arbitrary limit, but we need something */
>> + if (iov.iov_len > SZ_1G)
>> + goto err;
>> +
>> + ubuf = (unsigned long) iov.iov_base;
>> + end = (ubuf + iov.iov_len + PAGE_SIZE - 1) >> PAGE_SHIFT;
>> + start = ubuf >> PAGE_SHIFT;
>> + nr_pages = end - start;
>> +
>> + if (ctx->account_mem) {
>> + ret = io_account_mem(ctx->user, nr_pages);
>> + if (ret)
>> + goto err;
>> + }
>> +
>> + ret = 0;
>> + if (!pages || nr_pages > got_pages) {
>
> Nit: No need to check for `!pages` as long as `pages` and `got_pages`
> are synchronized (which guarantees that `!pages` implies
> `got_pages==0`).
I just prefer it that way, less confusion and past history this always
confuses the compiler and then we have to deal with a bogus warning.
>> + kfree(vmas);
>> + kfree(pages);
>> + pages = kmalloc_array(nr_pages, sizeof(struct page *),
>> + GFP_KERNEL);
>> + vmas = kmalloc_array(nr_pages,
>> + sizeof(struct vma_area_struct *),
>
> typo: s/vma_area_struct/vm_area_struct/
Fixed, thanks.
>> + GFP_KERNEL);
>> + if (!pages || !vmas) {
>> + ret = -ENOMEM;
>> + if (ctx->account_mem)
>> + io_unaccount_mem(ctx->user, nr_pages);
>> + goto err;
>> + }
>> + got_pages = nr_pages;
>> + }
>> +
>> + imu->bvec = kmalloc_array(nr_pages, sizeof(struct bio_vec),
>> + GFP_KERNEL);
>> + ret = -ENOMEM;
>> + if (!imu->bvec) {
>> + if (ctx->account_mem)
>> + io_unaccount_mem(ctx->user, nr_pages);
>> + goto err;
>> + }
>> +
>> + ret = 0;
>> + down_read(¤t->mm->mmap_sem);
>> + pret = get_user_pages_longterm(ubuf, nr_pages, FOLL_WRITE,
>> + pages, vmas);
>> + if (pret == nr_pages) {
>> + /* don't support file backed memory */
>> + for (j = 0; j < nr_pages; j++) {
>> + struct vm_area_struct *vma = vmas[j];
>> +
>> + if (vma->vm_file &&
>> + !is_file_hugepages(vma->vm_file)) {
>> + ret = -EOPNOTSUPP;
>> + break;
>> + }
>> + }
>> + } else {
>> + ret = pret < 0 ? pret : -EFAULT;
>> + }
>> + up_read(¤t->mm->mmap_sem);
>> + if (ret) {
>> + /*
>> + * if we did partial map, or found file backed vmas,
>> + * release any pages we did get
>> + */
>> + if (pret > 0) {
>> + for (j = 0; j < pret; j++)
>> + put_page(pages[j]);
>> + }
>> + if (ctx->account_mem)
>> + io_unaccount_mem(ctx->user, nr_pages);
>> + goto err;
>> + }
>> +
>> + off = ubuf & ~PAGE_MASK;
>> + size = iov.iov_len;
>> + for (j = 0; j < nr_pages; j++) {
>> + size_t vec_len;
>> +
>> + vec_len = min_t(size_t, size, PAGE_SIZE - off);
>> + imu->bvec[j].bv_page = pages[j];
>> + imu->bvec[j].bv_len = vec_len;
>> + imu->bvec[j].bv_offset = off;
>> + off = 0;
>> + size -= vec_len;
>> + }
>> + /* store original address for later verification */
>> + imu->ubuf = ubuf;
>> + imu->len = iov.iov_len;
>> + imu->nr_bvecs = nr_pages;
>> + }
>> + kfree(pages);
>> + kfree(vmas);
>> + ctx->nr_user_bufs = nr_args;
>> + return 0;
>> +err:
>> + kfree(pages);
>> + kfree(vmas);
>> + io_sqe_buffer_unregister(ctx);
>
> io_sqe_buffer_unregister() gets rid of elements up to
> ctx->nr_user_bufs, but as far as I can tell, ctx->nr_user_bufs is
> always zero here. I think that's going to cause a reference leak.
Fixed, thanks.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCHv6 07/10] acpi/hmat: Register processor domain to its memory
From: Dan Williams @ 2019-02-22 19:21 UTC (permalink / raw)
To: Keith Busch
Cc: Rafael J. Wysocki, Linux Kernel Mailing List,
ACPI Devel Maling List, Linux Memory Management List, Linux API,
Greg Kroah-Hartman, Dave Hansen
In-Reply-To: <20190222184831.GF10237@localhost.localdomain>
On Fri, Feb 22, 2019 at 10:48 AM Keith Busch <keith.busch@intel.com> wrote:
>
> On Wed, Feb 20, 2019 at 11:02:01PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Feb 14, 2019 at 6:10 PM Keith Busch <keith.busch@intel.com> wrote:
> > > config ACPI_HMAT
> > > bool "ACPI Heterogeneous Memory Attribute Table Support"
> > > depends on ACPI_NUMA
> > > + select HMEM_REPORTING
> >
> > If you want to do this here, I'm not sure that defining HMEM_REPORTING
> > as a user-selectable option is a good idea. In particular, I don't
> > really think that setting ACPI_HMAT without it makes a lot of sense.
> > Apart from this, the patch looks reasonable to me.
>
> I'm trying to implement based on the feedback, but I'm a little confused.
>
> As I have it at the moment, HMEM_REPORTING is not user-prompted, so
> another option needs to turn it on. I have ACPI_HMAT do that here.
>
> So when you say it's a bad idea to make HMEM_REPORTING user selectable,
> isn't it already not user selectable?
>
> If I do it the other way around, that's going to make HMEM_REPORTING
> complicated if a non-ACPI implementation wants to report HMEM
> properties.
Agree. If a platform supports these HMEM properties then they should
be reported. ACPI_HMAT is that opt-in for ACPI based platforms, and
other archs can do something similar. It's not clear that one would
ever want to opt-in to HMAT support and opt-out of reporting any of it
to userspace.
^ permalink raw reply
* Re: [RFC PATCH 0/6] Allow setting file birth time with utimensat()
From: Omar Sandoval @ 2019-02-22 19:00 UTC (permalink / raw)
To: Dave Chinner
Cc: Andreas Dilger, linux-fsdevel, Al Viro, kernel-team, Linux API,
linux-btrfs, linux-ext4, linux-f2fs-devel, linux-xfs,
Theodore Ts'o, Jaegeuk Kim, Steve French
In-Reply-To: <20190218221820.GF14116@dastard>
On Tue, Feb 19, 2019 at 09:18:20AM +1100, Dave Chinner wrote:
> On Sat, Feb 16, 2019 at 06:57:45PM -0700, Andreas Dilger wrote:
> > While it may be a bit of a stretch to call this "forensic evidence", making
>
> We do forensic analysis of corrupt filesystems looking for evidence
> of what went wrong, not just looking for evidence of what happened
> on systems that have been broken into.
>
> > it hard to change from except via total root compromise by a skilled hacker
> > is very useful.
>
> *nod*.
>
> > If this were to go in (which I'm not in favour of), then there would need to
> > be a CONFIG and/or runtime knob to turn it off (or better to only turn it on),
> > similar to how FIPS and other security options can only go in one direction.
>
> The problem here is that "inode birth time" is being conflated with
> "user document creation time". These two things are very different.
>
> i.e. One is filesystem internal information and is not related to
> when the original copy of the data in the file was created, the
> other is user specified metadata that is related to the file data
> contents and needs to travel with the data, not the filesystem.
>
> IMO, trying to make one on-disk field hold two different types of
> information defeats one or the other purpose, and nobody knows which
> one the field stores for any given file.
>
> I'd suggest that "authored date" should be a generic system xattr so
> most filesystems support it, not just those that have a birth time
> field on disk. Sure, modify it through utimesat() and expose it
> through statx() (as authored time, not birth time), but store it a
> system xattr rather than an internal filesystem metadata field that
> requires was never intended to be user modifiable.
It seems that this is the general consensus, so I'll look into
implementing this functionality as an xattr.
^ permalink raw reply
* Re: [PATCHv6 07/10] acpi/hmat: Register processor domain to its memory
From: Keith Busch @ 2019-02-22 18:48 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
Linux Memory Management List, Linux API, Greg Kroah-Hartman,
Dave Hansen, Dan Williams
In-Reply-To: <CAJZ5v0gjv0DZvYMTPBLnUmMtu8=g0zFd4x-cpP11Kzv+6XCwUw@mail.gmail.com>
On Wed, Feb 20, 2019 at 11:02:01PM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 14, 2019 at 6:10 PM Keith Busch <keith.busch@intel.com> wrote:
> > config ACPI_HMAT
> > bool "ACPI Heterogeneous Memory Attribute Table Support"
> > depends on ACPI_NUMA
> > + select HMEM_REPORTING
>
> If you want to do this here, I'm not sure that defining HMEM_REPORTING
> as a user-selectable option is a good idea. In particular, I don't
> really think that setting ACPI_HMAT without it makes a lot of sense.
> Apart from this, the patch looks reasonable to me.
I'm trying to implement based on the feedback, but I'm a little confused.
As I have it at the moment, HMEM_REPORTING is not user-prompted, so
another option needs to turn it on. I have ACPI_HMAT do that here.
So when you say it's a bad idea to make HMEM_REPORTING user selectable,
isn't it already not user selectable?
If I do it the other way around, that's going to make HMEM_REPORTING
complicated if a non-ACPI implementation wants to report HMEM
properties.
^ permalink raw reply
* Re: [PATCHv6 06/10] node: Add memory-side caching attributes
From: Dan Williams @ 2019-02-22 18:20 UTC (permalink / raw)
To: Keith Busch
Cc: Brice Goglin, Linux Kernel Mailing List, Linux ACPI, Linux MM,
Linux API, Greg Kroah-Hartman, Rafael Wysocki, Dave Hansen
In-Reply-To: <20190222180944.GD10237@localhost.localdomain>
On Fri, Feb 22, 2019 at 10:09 AM Keith Busch <keith.busch@intel.com> wrote:
>
> On Fri, Feb 22, 2019 at 11:12:38AM +0100, Brice Goglin wrote:
> > Le 14/02/2019 à 18:10, Keith Busch a écrit :
> > > +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/size
> > > +Date: December 2018
> > > +Contact: Keith Busch <keith.busch@intel.com>
> > > +Description:
> > > + The size of this memory side cache in bytes.
> >
> >
> > Hello Keith,
> >
> > CPU-side cache size is reported in kilobytes:
> >
> > $ cat
> > /sys/devices/system/cpu/cpu0/cache/index*/size
> >
> > 32K
> > 32K
> > 256K
> > 4096K
> >
> > Can you do the same of memory-side caches instead of reporting bytes?
>
> Ok, will do.
Ugh, please no. Don't optimize sysfs for human consumption. That 'K'
now needs to be parsed.
^ permalink raw reply
* Re: [PATCHv6 06/10] node: Add memory-side caching attributes
From: Keith Busch @ 2019-02-22 18:13 UTC (permalink / raw)
To: Brice Goglin
Cc: linux-kernel, linux-acpi, linux-mm, linux-api, Greg Kroah-Hartman,
Rafael Wysocki, Dave Hansen, Dan Williams
In-Reply-To: <16221be9-2f60-3a39-fd6c-5299cd94dc02@inria.fr>
On Fri, Feb 22, 2019 at 11:22:12AM +0100, Brice Goglin wrote:
> Le 14/02/2019 à 18:10, Keith Busch a écrit :
> > +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/associativity
> > +Date: December 2018
> > +Contact: Keith Busch <keith.busch@intel.com>
> > +Description:
> > + The caches associativity: 0 for direct mapped, non-zero if
> > + indexed.
>
>
> Should we rename "associativity" into "indexing" or something else?
>
> When I see "associativity" that contains 0, I tend to interpret this as
> the associativity value itself, which would mean fully-associative here
> (as in CPU-side cache "ways_of_associativity" attribute), while actually
> 0 means direct-mapped (ie 1-associative) with yout semantics.
>
> Brice
Yes, that's a good suggestion.
^ permalink raw reply
* Re: [PATCHv6 06/10] node: Add memory-side caching attributes
From: Keith Busch @ 2019-02-22 18:09 UTC (permalink / raw)
To: Brice Goglin
Cc: linux-kernel, linux-acpi, linux-mm, linux-api, Greg Kroah-Hartman,
Rafael Wysocki, Dave Hansen, Dan Williams
In-Reply-To: <29336223-b86e-3aca-ee5a-276d1c404b96@inria.fr>
On Fri, Feb 22, 2019 at 11:12:38AM +0100, Brice Goglin wrote:
> Le 14/02/2019 à 18:10, Keith Busch a écrit :
> > +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/size
> > +Date: December 2018
> > +Contact: Keith Busch <keith.busch@intel.com>
> > +Description:
> > + The size of this memory side cache in bytes.
>
>
> Hello Keith,
>
> CPU-side cache size is reported in kilobytes:
>
> $ cat
> /sys/devices/system/cpu/cpu0/cache/index*/size
>
> 32K
> 32K
> 256K
> 4096K
>
> Can you do the same of memory-side caches instead of reporting bytes?
Ok, will do.
^ permalink raw reply
* Re: [RFC PATCH 0/6] Allow setting file birth time with utimensat()
From: David Sterba @ 2019-02-22 15:02 UTC (permalink / raw)
To: Omar Sandoval
Cc: linux-fsdevel, Al Viro, kernel-team, linux-api, linux-btrfs,
linux-ext4, linux-f2fs-devel, linux-xfs
In-Reply-To: <cover.1550136164.git.osandov@fb.com>
On Thu, Feb 14, 2019 at 02:00:07AM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> Since statx was added in 4.11, userspace has had an interface for
> reading btime (file creation time), but no way to set it. This RFC patch
> series adds support for changing btime with utimensat(). Patch 1 adds
> the VFS infrastructure, patch 2 adds the support to utimensat() with a
> new flag, and the rest of the patches add filesystem support; I excluded
> CIFS for now because I don't have a CIFS setup to test it on.
>
> Updating btime is useful for at least a couple of use cases:
>
> - Backup/restore programs (my motivation for this feature is btrfs send)
> - File servers which interoperate with operating systems that allow
> updating file creation time, including Mac OS [1] and Windows [2]
I don't have anything new to add to what has been said in the thread.
The creation time is property of the filesystem and if user wants to
track additional metadata, then as external attributes.
^ permalink raw reply
* Re: [PATCHSET v15] io_uring IO interface
From: Marek Majkowski @ 2019-02-22 15:01 UTC (permalink / raw)
To: Jens Axboe
Cc: Avi Kivity, hch, Jann Horn, jmoyer, linux-aio, linux-api,
linux-block, viro
In-Reply-To: <e9b96cd8-71a1-d5bc-e3cf-8248f9654deb@kernel.dk>
On Thu, Feb 21, 2019 at 6:48 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 2/21/19 5:10 AM, Marek Majkowski wrote:
> >> From: Jens Axboe <axboe@kernel.dk>
> >> Subject: [PATCHSET v15] io_uring IO interface
> >> Message-ID: <20190211190049.7888-1-axboe@kernel.dk> (raw)
> >>
> >> Some final tweaks, mostly cosmetic, but also two important fixes:
> >>
> >> 1) Ensure that we account the skb appropriately against the socket.
> >> Some network config options apparently return is an skb with
> >> ->truesize != 0 when allocated with a size of 0, ensure we add
> >> those as references against sock->sk_wmem_alloc. Reported by
> >> Matt Mullins.
> >
> > Jens,
> >
> > I tried using io_uring with network sockets. It seem to be doing the
> > right thing. One bit is missing though: "flags" as in recv(2).
> >
> > In perfect world I would like to specify at least:
> > - MSG_DONTWAIT
> > - MSG_WAITALL
> > - MSG_NOSIGNAL
> >
> > Right now, unless I'm missing something, io_uring_sqe doesn't have a
> > place where we could store these. "flags" is needed for any
> > non-trivial network I/O.
>
> We have flags for sqes, depending on the type. You can add to the
> union that already holds rw_flags/fsync_flags/poll_events? There's
> also a (smaller) flags field that applies for all types, which
> currently only holds the fixed file flag.
The "sqe->flags" right now is used by the IOSQE_FIXED_FILE which has
the same value as MSG_OOB.
Sticking recv/send flags into the "rw_flags" union perhaps could work,
barring the discussion about naming. The obvious names don't make
sense. recv_flags, send_flags or socket_flags don't sound right.
If we tried to add networking stuff to io_uring (for batchinig and async), then:
- send()/recv() could work, only needs the "flags" field
- sendmsg()/recvmsg() likewise
- sendto()/recvfrom() require two more pointers: (struct sockaddr
*dest_addr, socklen_t addrlen)
- sendmmsg() / recvmmsg() are perhaps irrelevant
Non-blocking stuff like socket(), setsockopt(), bind() perhaps don't
need to be considered, although could benefit from batching.
Not sure what to think about connect() and accept(). In the
prehistoric days there seem to have been an attempt to add socket
things to libaio struct iocb. See:
https://code.woboq.org/linux/include/libaio.h.html#iocb::(anonymous)::saddr
struct iocb {
...
union {
...
struct io_iocb_sockaddr saddr;
} u;
};
Are there chances of reserving space for two pointers in io_uring_sqe,
which could be used for sendto/recvfrom/accept if we decided to add
more network support?
Cheers,
Marek
> If you're talking on a per-syscall type of flag, io_uring_enter(2)
> does take a flags member.
>
> --
> Jens Axboe
>
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [RFC PATCH] mm,mremap: Bail out earlier in mremap_to under map pressure
From: Kirill A. Shutemov @ 2019-02-22 13:01 UTC (permalink / raw)
To: Oscar Salvador
Cc: linux-mm, linux-kernel, linux-api, hughd, vbabka, joel, jglisse,
yang.shi, mgorman
In-Reply-To: <20190221085406.10852-1-osalvador@suse.de>
On Thu, Feb 21, 2019 at 09:54:06AM +0100, Oscar Salvador wrote:
> When using mremap() syscall in addition to MREMAP_FIXED flag,
> mremap() calls mremap_to() which does the following:
>
> 1) unmaps the destination region where we are going to move the map
> 2) If the new region is going to be smaller, we unmap the last part
> of the old region
>
> Then, we will eventually call move_vma() to do the actual move.
>
> move_vma() checks whether we are at least 4 maps below max_map_count
> before going further, otherwise it bails out with -ENOMEM.
> The problem is that we might have already unmapped the vma's in steps
> 1) and 2), so it is not possible for userspace to figure out the state
> of the vma's after it gets -ENOMEM, and it gets tricky for userspace
> to clean up properly on error path.
>
> While it is true that we can return -ENOMEM for more reasons
> (e.g: see may_expand_vm() or move_page_tables()), I think that we can
> avoid this scenario in concret if we check early in mremap_to() if the
> operation has high chances to succeed map-wise.
>
> Should not be that the case, we can bail out before we even try to unmap
> anything, so we make sure the vma's are left untouched in case we are likely
> to be short of maps.
>
> The thumb-rule now is to rely on the worst-scenario case we can have.
> That is when both vma's (old region and new region) are going to be split
> in 3, so we get two more maps to the ones we already hold (one per each).
> If current map count + 2 maps still leads us to 4 maps below the threshold,
> we are going to pass the check in move_vma().
>
> Of course, this is not free, as it might generate false positives when it is
> true that we are tight map-wise, but the unmap operation can release several
> vma's leading us to a good state.
>
> Because of that I am sending this as a RFC.
> Another approach was also investigated [1], but it may be too much hassle
> for what it brings.
I believe we don't need the check in move_vma() with this patch. Or do we?
>
> [1] https://lore.kernel.org/lkml/20190219155320.tkfkwvqk53tfdojt@d104.suse.de/
>
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
> mm/mremap.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 3320616ed93f..e3edef6b7a12 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -516,6 +516,23 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
> if (addr + old_len > new_addr && new_addr + new_len > addr)
> goto out;
>
> + /*
> + * move_vma() need us to stay 4 maps below the threshold, otherwise
> + * it will bail out at the very beginning.
> + * That is a problem if we have already unmaped the regions here
> + * (new_addr, and old_addr), because userspace will not know the
> + * state of the vma's after it gets -ENOMEM.
> + * So, to avoid such scenario we can pre-compute if the whole
> + * operation has high chances to success map-wise.
> + * Worst-scenario case is when both vma's (new_addr and old_addr) get
> + * split in 3 before unmaping it.
> + * That means 2 more maps (1 for each) to the ones we already hold.
> + * Check whether current map count plus 2 still leads us to 4 maps below
> + * the threshold, otherwise return -ENOMEM here to be more safe.
> + */
> + if ((mm->map_count + 2) >= sysctl_max_map_count - 3)
Nit: redundant parentheses around 'mm->map_count + 2'.
> + return -ENOMEM;
> +
> ret = do_munmap(mm, new_addr, new_len, uf_unmap_early);
> if (ret)
> goto out;
> --
> 2.13.7
>
--
Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCHv6 06/10] node: Add memory-side caching attributes
From: Brice Goglin @ 2019-02-22 10:22 UTC (permalink / raw)
To: Keith Busch, linux-kernel, linux-acpi, linux-mm, linux-api
Cc: Greg Kroah-Hartman, Rafael Wysocki, Dave Hansen, Dan Williams
In-Reply-To: <20190214171017.9362-7-keith.busch@intel.com>
Le 14/02/2019 à 18:10, Keith Busch a écrit :
> System memory may have caches to help improve access speed to frequently
> requested address ranges. While the system provided cache is transparent
> to the software accessing these memory ranges, applications can optimize
> their own access based on cache attributes.
>
> Provide a new API for the kernel to register these memory-side caches
> under the memory node that provides it.
>
> The new sysfs representation is modeled from the existing cpu cacheinfo
> attributes, as seen from /sys/devices/system/cpu/<cpu>/cache/. Unlike CPU
> cacheinfo though, the node cache level is reported from the view of the
> memory. A higher level number is nearer to the CPU, while lower levels
> are closer to the last level memory.
>
> The exported attributes are the cache size, the line size, associativity,
> and write back policy, and add the attributes for the system memory
> caches to sysfs stable documentation.
>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
> Documentation/ABI/stable/sysfs-devices-node | 35 +++++++
> drivers/base/node.c | 151 ++++++++++++++++++++++++++++
> include/linux/node.h | 34 +++++++
> 3 files changed, 220 insertions(+)
>
> diff --git a/Documentation/ABI/stable/sysfs-devices-node b/Documentation/ABI/stable/sysfs-devices-node
> index cd64b62152ba..5c88cb9ca14e 100644
> --- a/Documentation/ABI/stable/sysfs-devices-node
> +++ b/Documentation/ABI/stable/sysfs-devices-node
> @@ -143,3 +143,38 @@ Contact: Keith Busch <keith.busch@intel.com>
> Description:
> This node's write latency in nanoseconds when access
> from nodes found in this class's linked initiators.
> +
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The directory containing attributes for the memory-side cache
> + level 'Y'.
> +
> + The caches associativity: 0 for direct mapped, non-zero if
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/associativity
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The caches associativity: 0 for direct mapped, non-zero if
> + indexed.
Should we rename "associativity" into "indexing" or something else?
When I see "associativity" that contains 0, I tend to interpret this as
the associativity value itself, which would mean fully-associative here
(as in CPU-side cache "ways_of_associativity" attribute), while actually
0 means direct-mapped (ie 1-associative) with yout semantics.
Brice
^ permalink raw reply
* Re: [PATCHv6 06/10] node: Add memory-side caching attributes
From: Brice Goglin @ 2019-02-22 10:12 UTC (permalink / raw)
To: Keith Busch, linux-kernel, linux-acpi, linux-mm, linux-api
Cc: Greg Kroah-Hartman, Rafael Wysocki, Dave Hansen, Dan Williams
In-Reply-To: <20190214171017.9362-7-keith.busch@intel.com>
Le 14/02/2019 à 18:10, Keith Busch a écrit :
> System memory may have caches to help improve access speed to frequently
> requested address ranges. While the system provided cache is transparent
> to the software accessing these memory ranges, applications can optimize
> their own access based on cache attributes.
>
> Provide a new API for the kernel to register these memory-side caches
> under the memory node that provides it.
>
> The new sysfs representation is modeled from the existing cpu cacheinfo
> attributes, as seen from /sys/devices/system/cpu/<cpu>/cache/. Unlike CPU
> cacheinfo though, the node cache level is reported from the view of the
> memory. A higher level number is nearer to the CPU, while lower levels
> are closer to the last level memory.
>
> The exported attributes are the cache size, the line size, associativity,
> and write back policy, and add the attributes for the system memory
> caches to sysfs stable documentation.
>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
> Documentation/ABI/stable/sysfs-devices-node | 35 +++++++
> drivers/base/node.c | 151 ++++++++++++++++++++++++++++
> include/linux/node.h | 34 +++++++
> 3 files changed, 220 insertions(+)
>
> diff --git a/Documentation/ABI/stable/sysfs-devices-node b/Documentation/ABI/stable/sysfs-devices-node
> index cd64b62152ba..5c88cb9ca14e 100644
> --- a/Documentation/ABI/stable/sysfs-devices-node
> +++ b/Documentation/ABI/stable/sysfs-devices-node
> @@ -143,3 +143,38 @@ Contact: Keith Busch <keith.busch@intel.com>
> Description:
> This node's write latency in nanoseconds when access
> from nodes found in this class's linked initiators.
> +
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The directory containing attributes for the memory-side cache
> + level 'Y'.
> +
> + The caches associativity: 0 for direct mapped, non-zero if
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/associativity
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The caches associativity: 0 for direct mapped, non-zero if
> + indexed.
> +
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/line_size
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The number of bytes accessed from the next cache level on a
> + cache miss.
> +
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/size
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The size of this memory side cache in bytes.
Hello Keith,
CPU-side cache size is reported in kilobytes:
$ cat
/sys/devices/system/cpu/cpu0/cache/index*/size
32K
32K
256K
4096K
Can you do the same of memory-side caches instead of reporting bytes?
Thanks
Brice
^ permalink raw reply
* [PATCH 3/3] selftests/bpf: add tests for bpf_sk_check_syncookie
From: Lorenz Bauer @ 2019-02-22 9:50 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: linux-api, Lorenz Bauer
In-Reply-To: <20190222095057.9442-1-lmb@cloudflare.com>
Add tests which verify that the new helper works for both IPv4 and
IPv6, by forcing SYN cookies to always on. Use a new network namespace
to avoid clobbering the global SYN cookie settings.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/Makefile | 5 +-
tools/testing/selftests/bpf/bpf_helpers.h | 3 +
.../bpf/progs/test_sk_syncookie_kern.c | 119 ++++++++++
.../selftests/bpf/test_sk_syncookie.sh | 81 +++++++
.../selftests/bpf/test_sk_syncookie_user.c | 212 ++++++++++++++++++
6 files changed, 419 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
create mode 100755 tools/testing/selftests/bpf/test_sk_syncookie.sh
create mode 100644 tools/testing/selftests/bpf/test_sk_syncookie_user.c
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index e47168d1257d..44b2853b8534 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -29,4 +29,5 @@ test_netcnt
test_section_names
test_tcpnotify_user
test_libbpf
+test_sk_syncookie_user
alu32
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index ccffaa0a0787..4ad1f7a915b3 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -51,7 +51,8 @@ TEST_PROGS := test_kmod.sh \
test_skb_cgroup_id.sh \
test_flow_dissector.sh \
test_xdp_vlan.sh \
- test_lwt_ip_encap.sh
+ test_lwt_ip_encap.sh \
+ test_sk_syncookie.sh
TEST_PROGS_EXTENDED := with_addr.sh \
with_tunnels.sh \
@@ -60,7 +61,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
# Compile but not part of 'make run_tests'
TEST_GEN_PROGS_EXTENDED = test_libbpf_open test_sock_addr test_skb_cgroup_id_user \
- flow_dissector_load test_flow_dissector
+ flow_dissector_load test_flow_dissector test_sk_syncookie_user
include ../lib.mk
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index d9999f1ed1d2..b80cec7af445 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -166,6 +166,9 @@ static struct bpf_sock *(*bpf_sk_lookup_udp)(void *ctx,
(void *) BPF_FUNC_sk_lookup_udp;
static int (*bpf_sk_release)(struct bpf_sock *sk) =
(void *) BPF_FUNC_sk_release;
+static int (*bpf_sk_check_syncookie)(struct bpf_sock *sk,
+ void *ip, int ip_len, void *tcp, int tcp_len) =
+ (void *) BPF_FUNC_sk_check_syncookie;
static int (*bpf_skb_vlan_push)(void *ctx, __be16 vlan_proto, __u16 vlan_tci) =
(void *) BPF_FUNC_skb_vlan_push;
static int (*bpf_skb_vlan_pop)(void *ctx) =
diff --git a/tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c b/tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
new file mode 100644
index 000000000000..4918457efc1e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+// Copyright (c) 2019 Cloudflare
+
+#include <string.h>
+
+#include <linux/bpf.h>
+#include <linux/pkt_cls.h>
+#include <linux/if_ether.h>
+#include <linux/in.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <sys/socket.h>
+#include <linux/tcp.h>
+
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+
+struct bpf_map_def SEC("maps") results = {
+ .type = BPF_MAP_TYPE_ARRAY,
+ .key_size = sizeof(__u32),
+ .value_size = sizeof(__u64),
+ .max_entries = 1,
+};
+
+static __always_inline void check_syncookie(void *ctx, void *data,
+ void *data_end)
+{
+ struct bpf_sock_tuple tup;
+ struct bpf_sock *sk;
+ struct ethhdr *ethh;
+ struct iphdr *ipv4h;
+ struct ipv6hdr *ipv6h;
+ struct tcphdr *tcph;
+ int ret;
+ __u32 key = 0;
+ __u64 value = 1;
+
+ ethh = data;
+ if (ethh + 1 > data_end)
+ return;
+
+ switch (bpf_ntohs(ethh->h_proto)) {
+ case ETH_P_IP:
+ ipv4h = data + sizeof(struct ethhdr);
+ if (ipv4h + 1 > data_end)
+ return;
+
+ tcph = data + sizeof(struct ethhdr) + sizeof(struct iphdr);
+ if (tcph + 1 > data_end)
+ return;
+
+ tup.ipv4.saddr = ipv4h->saddr;
+ tup.ipv4.daddr = ipv4h->daddr;
+ tup.ipv4.sport = tcph->source;
+ tup.ipv4.dport = tcph->dest;
+
+ sk = bpf_sk_lookup_tcp(ctx, &tup, sizeof(tup.ipv4),
+ BPF_F_CURRENT_NETNS, 0);
+ if (!sk)
+ return;
+
+ ret = bpf_sk_check_syncookie(sk, ipv4h, sizeof(*ipv4h),
+ tcph, sizeof(*tcph));
+ break;
+
+ case ETH_P_IPV6:
+ ipv6h = data + sizeof(struct ethhdr);
+ if (ipv6h + 1 > data_end)
+ return;
+
+ if (ipv6h->nexthdr != IPPROTO_TCP)
+ return;
+
+ tcph = data + sizeof(struct ethhdr) + sizeof(struct ipv6hdr);
+ if (tcph + 1 > data_end)
+ return;
+
+ memcpy(tup.ipv6.saddr, &ipv6h->saddr, sizeof(tup.ipv6.saddr));
+ memcpy(tup.ipv6.daddr, &ipv6h->daddr, sizeof(tup.ipv6.daddr));
+ tup.ipv6.sport = tcph->source;
+ tup.ipv6.dport = tcph->dest;
+
+ sk = bpf_sk_lookup_tcp(ctx, &tup, sizeof(tup.ipv6),
+ BPF_F_CURRENT_NETNS, 0);
+ if (!sk)
+ return;
+
+ ret = bpf_sk_check_syncookie(sk, ipv6h, sizeof(*ipv6h),
+ tcph, sizeof(*tcph));
+ break;
+
+ default:
+ return;
+ }
+
+ if (ret == 0)
+ bpf_map_update_elem(&results, &key, &value, 0);
+
+ bpf_sk_release(sk);
+}
+
+SEC("clsact/check_syncookie")
+int check_syncookie_clsact(struct __sk_buff *skb)
+{
+ check_syncookie(skb, (void *)(long)skb->data,
+ (void *)(long)skb->data_end);
+ return TC_ACT_OK;
+}
+
+SEC("xdp/check_syncookie")
+int check_syncookie_xdp(struct xdp_md *ctx)
+{
+ check_syncookie(ctx, (void *)(long)ctx->data,
+ (void *)(long)ctx->data_end);
+ return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_sk_syncookie.sh b/tools/testing/selftests/bpf/test_sk_syncookie.sh
new file mode 100755
index 000000000000..429ca8c04c5e
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_sk_syncookie.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2018 Facebook
+# Copyright (c) 2019 Cloudflare
+
+set -eu
+
+wait_for_ip()
+{
+ local _i
+ printf "Wait for IP %s to become available " "$1"
+ for _i in $(seq ${MAX_PING_TRIES}); do
+ printf "."
+ if ns1_exec ping -c 1 -W 1 "$1" >/dev/null 2>&1; then
+ echo " OK"
+ return
+ fi
+ sleep 1
+ done
+ echo 1>&2 "ERROR: Timeout waiting for test IP to become available."
+ exit 1
+}
+
+get_prog_id()
+{
+ awk '/ id / {sub(/.* id /, "", $0); print($1)}'
+}
+
+ns1_exec()
+{
+ ip netns exec ns1 "$@"
+}
+
+setup()
+{
+ ip netns add ns1
+ ns1_exec ip link set lo up
+
+ ns1_exec sysctl -w net.ipv4.tcp_syncookies=2
+
+ wait_for_ip 127.0.0.1
+ wait_for_ip ::1
+}
+
+cleanup()
+{
+ ip netns del ns1 2>/dev/null || :
+}
+
+main()
+{
+ trap cleanup EXIT 2 3 6 15
+ setup
+
+ printf "Testing clsact..."
+ ns1_exec tc qdisc add dev "${TEST_IF}" clsact
+ ns1_exec tc filter add dev "${TEST_IF}" ingress \
+ bpf obj "${BPF_PROG_OBJ}" sec "${CLSACT_SECTION}" da
+
+ BPF_PROG_ID=$(ns1_exec tc filter show dev "${TEST_IF}" ingress | \
+ get_prog_id)
+ ns1_exec "${PROG}" "${BPF_PROG_ID}"
+ ns1_exec tc qdisc del dev "${TEST_IF}" clsact
+
+ printf "Testing XDP..."
+ ns1_exec ip link set "${TEST_IF}" xdp \
+ object "${BPF_PROG_OBJ}" section "${XDP_SECTION}"
+ BPF_PROG_ID=$(ns1_exec ip link show "${TEST_IF}" | get_prog_id)
+ ns1_exec "${PROG}" "${BPF_PROG_ID}"
+}
+
+DIR=$(dirname $0)
+TEST_IF=lo
+MAX_PING_TRIES=5
+BPF_PROG_OBJ="${DIR}/test_sk_syncookie_kern.o"
+CLSACT_SECTION="clsact/check_syncookie"
+XDP_SECTION="xdp/check_syncookie"
+BPF_PROG_ID=0
+PROG="${DIR}/test_sk_syncookie_user"
+
+main
diff --git a/tools/testing/selftests/bpf/test_sk_syncookie_user.c b/tools/testing/selftests/bpf/test_sk_syncookie_user.c
new file mode 100644
index 000000000000..87829c86c746
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_sk_syncookie_user.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+// Copyright (c) 2019 Cloudflare
+
+#include <string.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "bpf_rlimit.h"
+#include "cgroup_helpers.h"
+
+static int start_server(const struct sockaddr *addr, socklen_t len)
+{
+ int fd;
+
+ fd = socket(addr->sa_family, SOCK_STREAM, 0);
+ if (fd == -1) {
+ log_err("Failed to create server socket");
+ goto out;
+ }
+
+ if (bind(fd, addr, len) == -1) {
+ log_err("Failed to bind server socket");
+ goto close_out;
+ }
+
+ if (listen(fd, 128) == -1) {
+ log_err("Failed to listen on server socket");
+ goto close_out;
+ }
+
+ goto out;
+
+close_out:
+ close(fd);
+ fd = -1;
+out:
+ return fd;
+}
+
+static int connect_to_server(int server_fd)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd = -1;
+
+ if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
+ log_err("Failed to get server addr");
+ goto out;
+ }
+
+ fd = socket(addr.ss_family, SOCK_STREAM, 0);
+ if (fd == -1) {
+ log_err("Failed to create client socket");
+ goto out;
+ }
+
+ if (connect(fd, (const struct sockaddr *)&addr, len) == -1) {
+ log_err("Fail to connect to server");
+ goto close_out;
+ }
+
+ goto out;
+
+close_out:
+ close(fd);
+ fd = -1;
+out:
+ return fd;
+}
+
+static int get_map_fd_by_prog_id(int prog_id)
+{
+ struct bpf_prog_info info = {};
+ __u32 info_len = sizeof(info);
+ __u32 map_ids[1];
+ int prog_fd = -1;
+ int map_fd = -1;
+
+ prog_fd = bpf_prog_get_fd_by_id(prog_id);
+ if (prog_fd < 0) {
+ log_err("Failed to get fd by prog id %d", prog_id);
+ goto err;
+ }
+
+ info.nr_map_ids = 1;
+ info.map_ids = (__u64)(unsigned long)map_ids;
+
+ if (bpf_obj_get_info_by_fd(prog_fd, &info, &info_len)) {
+ log_err("Failed to get info by prog fd %d", prog_fd);
+ goto err;
+ }
+
+ if (!info.nr_map_ids) {
+ log_err("No maps found for prog fd %d", prog_fd);
+ goto err;
+ }
+
+ map_fd = bpf_map_get_fd_by_id(map_ids[0]);
+ if (map_fd < 0)
+ log_err("Failed to get fd by map id %d", map_ids[0]);
+err:
+ if (prog_fd >= 0)
+ close(prog_fd);
+ return map_fd;
+}
+
+static int run_test(int server_fd, int results_fd)
+{
+ int client = -1, srv_client = -1;
+ int ret = 0;
+ __u32 key = 0;
+ __u64 value = 0;
+
+ if (bpf_map_update_elem(results_fd, &key, &value, 0) < 0) {
+ log_err("Can't clear results");
+ goto err;
+ }
+
+ client = connect_to_server(server_fd);
+ if (client == -1)
+ goto err;
+
+ srv_client = accept(server_fd, NULL, 0);
+ if (srv_client == -1) {
+ log_err("Can't accept connection");
+ goto err;
+ }
+
+ if (bpf_map_lookup_elem(results_fd, &key, &value) < 0) {
+ log_err("Can't lookup result");
+ goto err;
+ }
+
+ if (value != 1) {
+ log_err("Didn't match syncookie: %llu", value);
+ goto err;
+ }
+
+ goto out;
+
+err:
+ ret = 1;
+out:
+ close(client);
+ close(srv_client);
+ return ret;
+}
+
+int main(int argc, char **argv)
+{
+ struct sockaddr_in addr4;
+ struct sockaddr_in6 addr6;
+ int server = -1;
+ int server_v6 = -1;
+ int results = -1;
+ int err = 0;
+
+ if (argc < 2) {
+ fprintf(stderr, "Usage: %s prog_id\n", argv[0]);
+ exit(1);
+ }
+
+ results = get_map_fd_by_prog_id(atoi(argv[1]));
+ if (results < 0) {
+ log_err("Can't get map");
+ goto err;
+ }
+
+ memset(&addr4, 0, sizeof(addr4));
+ addr4.sin_family = AF_INET;
+ addr4.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+ addr4.sin_port = 0;
+
+ memset(&addr6, 0, sizeof(addr6));
+ addr6.sin6_family = AF_INET6;
+ addr6.sin6_addr = in6addr_loopback;
+ addr6.sin6_port = 0;
+
+ server = start_server((const struct sockaddr *)&addr4, sizeof(addr4));
+ if (server == -1)
+ goto err;
+
+ server_v6 = start_server((const struct sockaddr *)&addr6,
+ sizeof(addr6));
+ if (server_v6 == -1)
+ goto err;
+
+ if (run_test(server, results))
+ goto err;
+
+ if (run_test(server_v6, results))
+ goto err;
+
+ printf("ok\n");
+ goto out;
+err:
+ err = 1;
+out:
+ close(server);
+ close(server_v6);
+ close(results);
+ return err;
+}
--
2.19.1
^ permalink raw reply related
* [PATCH 2/3] tools: sync changes to uapi/linux/bpf.h
From: Lorenz Bauer @ 2019-02-22 9:50 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: linux-api, Lorenz Bauer
In-Reply-To: <20190222095057.9442-1-lmb@cloudflare.com>
Pull in definitions for bpf_sk_check_syncookie.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/include/uapi/linux/bpf.h | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index bcdd2474eee7..bc2af87e9621 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2359,6 +2359,21 @@ union bpf_attr {
* Return
* A **struct bpf_tcp_sock** pointer on success, or NULL in
* case of failure.
+ *
+ * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
+ * Description
+ * Check whether iph and th contain a valid SYN cookie ACK for
+ * the listening socket in sk.
+ *
+ * iph points to the start of the IPv4 or IPv6 header, while
+ * iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
+ *
+ * th points to the start of the TCP header, while th_len contains
+ * sizeof(struct tcphdr).
+ *
+ * Return
+ * 0 if iph and th are a valid SYN cookie ACK, or a negative error
+ * otherwise.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2457,7 +2472,8 @@ union bpf_attr {
FN(spin_lock), \
FN(spin_unlock), \
FN(sk_fullsock), \
- FN(tcp_sock),
+ FN(tcp_sock), \
+ FN(sk_check_syncookie),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
--
2.19.1
^ permalink raw reply related
* [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: Lorenz Bauer @ 2019-02-22 9:50 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: linux-api, Lorenz Bauer
In-Reply-To: <20190222095057.9442-1-lmb@cloudflare.com>
Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
to a known connection. However, there is one corner case: no sockets are
created if SYN cookies are active. This means that the final ACK in the
3WHS is misclassified.
Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
and then check whether a packet is a valid SYN cookie ACK.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/uapi/linux/bpf.h | 18 ++++++++++-
net/core/filter.c | 68 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bcdd2474eee7..bc2af87e9621 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2359,6 +2359,21 @@ union bpf_attr {
* Return
* A **struct bpf_tcp_sock** pointer on success, or NULL in
* case of failure.
+ *
+ * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
+ * Description
+ * Check whether iph and th contain a valid SYN cookie ACK for
+ * the listening socket in sk.
+ *
+ * iph points to the start of the IPv4 or IPv6 header, while
+ * iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
+ *
+ * th points to the start of the TCP header, while th_len contains
+ * sizeof(struct tcphdr).
+ *
+ * Return
+ * 0 if iph and th are a valid SYN cookie ACK, or a negative error
+ * otherwise.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2457,7 +2472,8 @@ union bpf_attr {
FN(spin_lock), \
FN(spin_unlock), \
FN(sk_fullsock), \
- FN(tcp_sock),
+ FN(tcp_sock), \
+ FN(sk_check_syncookie),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
diff --git a/net/core/filter.c b/net/core/filter.c
index 85749f6ec789..9e68897cc7ed 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
.arg1_type = ARG_PTR_TO_SOCK_COMMON,
};
+BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
+ struct tcphdr *, th, u32, th_len)
+{
+#if IS_ENABLED(CONFIG_SYN_COOKIES)
+ u32 cookie;
+ int ret;
+
+ if (unlikely(th_len < sizeof(*th)))
+ return -EINVAL;
+
+ /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
+ if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
+ return -EINVAL;
+
+ if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
+ return -EINVAL;
+
+ if (!th->ack || th->rst)
+ return -ENOENT;
+
+ cookie = ntohl(th->ack_seq) - 1;
+
+ switch (sk->sk_family) {
+ case AF_INET:
+ if (unlikely(iph_len < sizeof(struct iphdr)))
+ return -EINVAL;
+
+ ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
+ break;
+
+#if IS_ENABLED(CONFIG_IPV6)
+ case AF_INET6:
+ if (unlikely(iph_len < sizeof(struct ipv6hdr)))
+ return -EINVAL;
+
+ ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
+ break;
+#endif /* CONFIG_IPV6 */
+
+ default:
+ return -EPROTONOSUPPORT;
+ }
+
+ if (ret > 0)
+ return 0;
+
+ return -ENOENT;
+#else
+ return -ENOTSUP;
+#endif
+}
+
+static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
+ .func = bpf_sk_check_syncookie,
+ .gpl_only = true,
+ .pkt_access = true,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_SOCKET,
+ .arg2_type = ARG_PTR_TO_MEM,
+ .arg3_type = ARG_CONST_SIZE,
+ .arg4_type = ARG_PTR_TO_MEM,
+ .arg5_type = ARG_CONST_SIZE,
+};
+
#endif /* CONFIG_INET */
bool bpf_helper_changes_pkt_data(void *func)
@@ -5678,6 +5742,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_release_proto;
case BPF_FUNC_tcp_sock:
return &bpf_tcp_sock_proto;
+ case BPF_FUNC_sk_check_syncookie:
+ return &bpf_sk_check_syncookie_proto;
#endif
default:
return bpf_base_func_proto(func_id);
@@ -5713,6 +5779,8 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_xdp_sk_lookup_tcp_proto;
case BPF_FUNC_sk_release:
return &bpf_sk_release_proto;
+ case BPF_FUNC_sk_check_syncookie:
+ return &bpf_sk_check_syncookie_proto;
#endif
default:
return bpf_base_func_proto(func_id);
--
2.19.1
^ permalink raw reply related
* [PATCH 0/3] Allow checking SYN cookies from XDP and tc cls act
From: Lorenz Bauer @ 2019-02-22 9:50 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: linux-api, Lorenz Bauer
This series adds a helper bpf_sk_check_syncookie, which allows checking
whether a packet is a valid SYN cookie for a given socket. This is
useful for implementing load-balancing approaches like glb-director [1]
or Beamer [2] in pure eBPF.
Specifically, we'd like to replace the functionality of the glb-redirect
kernel module by an XDP program or tc classifier.
1: https://github.com/github/glb-director
2: https://www.usenix.org/conference/nsdi18/presentation/olteanu
Lorenz Bauer (3):
bpf: add helper to check for a valid SYN cookie
tools: sync changes to uapi/linux/bpf.h
selftests/bpf: add tests for bpf_sk_check_syncookie
include/uapi/linux/bpf.h | 18 +-
net/core/filter.c | 68 ++++++
tools/include/uapi/linux/bpf.h | 18 +-
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/Makefile | 5 +-
tools/testing/selftests/bpf/bpf_helpers.h | 3 +
.../bpf/progs/test_sk_syncookie_kern.c | 119 ++++++++++
.../selftests/bpf/test_sk_syncookie.sh | 81 +++++++
.../selftests/bpf/test_sk_syncookie_user.c | 212 ++++++++++++++++++
9 files changed, 521 insertions(+), 4 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
create mode 100755 tools/testing/selftests/bpf/test_sk_syncookie.sh
create mode 100644 tools/testing/selftests/bpf/test_sk_syncookie_user.c
^ permalink raw reply
* Re: [RFC PATCH v3 07/18] fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY ioctl
From: Eric Biggers @ 2019-02-21 18:42 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-ext4, open list:ABI/API, linux-f2fs-devel, linux-fscrypt,
keyrings, linux-mtd, linux-crypto, linux-fsdevel, Satya Tangirala,
Paul Crowley
In-Reply-To: <2024630.T9XyBPH5Ub@blindfold>
On Thu, Feb 21, 2019 at 10:33:12AM +0100, Richard Weinberger wrote:
> Eric,
>
> Am Donnerstag, 21. Februar 2019, 06:49:39 CET schrieb Eric Biggers:
> > Hi Richard,
> >
> > On Thu, Feb 21, 2019 at 12:52:38AM +0100, Richard Weinberger wrote:
> > > On Wed, Feb 20, 2019 at 7:55 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > +#define FSCRYPT_FS_KEYRING_DESCRIPTION_SIZE \
> > > > + (CONST_STRLEN("fscrypt-") + FIELD_SIZEOF(struct super_block, s_id))
> > > > +
> > > > +#define FSCRYPT_MK_DESCRIPTION_SIZE (2 * FSCRYPT_KEY_DESCRIPTOR_SIZE + 1)
> > > > +
> > > > +static void format_fs_keyring_description(
> > > > + char description[FSCRYPT_FS_KEYRING_DESCRIPTION_SIZE],
> > > > + const struct super_block *sb)
> > > > +{
> > > > + sprintf(description, "fscrypt-%s", sb->s_id);
> > > > +}
> > >
> > > I fear ->s_id is not the right thing.
> > > For filesystems such as ext4 ->s_id is the name of the backing block device,
> > > so it is per filesysem instance unique.
> > > But this is not guaranteed. For UBIFS ->s_id is just "ubifs", always.
> > > So the names will clash.
> > >
> >
> > What name do you suggest using for UBIFS filesystems? The keyring name could be
> > set by the filesystem via a fscrypt_operations callback if needed.
>
> IMHO the BDI name should be used.
>
> > Note that the keyring name isn't particularly important, since the ioctls will
> > work regardless. But we might as well choose something logical, since the
> > keyring name will still show up in /proc/keys.
>
> I'm not done with reviewing your patches, but will it be possible to use keyctl?
> For the a unique name is helpful. :)
>
Not for adding keys, removing keys, or getting a key's status -- those are what
the ioctls are for.
See e.g. the discussion in patch 7 ("fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY
ioctl") for why the keyrings syscalls are a poor fit for fscrypt.
- Eric
^ permalink raw reply
* Re: [PATCHSET v15] io_uring IO interface
From: Jens Axboe @ 2019-02-21 17:48 UTC (permalink / raw)
To: Marek Majkowski
Cc: avi, hch, jannh, jmoyer, linux-aio, linux-api, linux-block, viro
In-Reply-To: <20190221121022.7867-1-marek@cloudflare.com>
On 2/21/19 5:10 AM, Marek Majkowski wrote:
>> From: Jens Axboe <axboe@kernel.dk>
>> Subject: [PATCHSET v15] io_uring IO interface
>> Message-ID: <20190211190049.7888-1-axboe@kernel.dk> (raw)
>>
>> Some final tweaks, mostly cosmetic, but also two important fixes:
>>
>> 1) Ensure that we account the skb appropriately against the socket.
>> Some network config options apparently return is an skb with
>> ->truesize != 0 when allocated with a size of 0, ensure we add
>> those as references against sock->sk_wmem_alloc. Reported by
>> Matt Mullins.
>
> Jens,
>
> I tried using io_uring with network sockets. It seem to be doing the
> right thing. One bit is missing though: "flags" as in recv(2).
>
> In perfect world I would like to specify at least:
> - MSG_DONTWAIT
> - MSG_WAITALL
> - MSG_NOSIGNAL
>
> Right now, unless I'm missing something, io_uring_sqe doesn't have a
> place where we could store these. "flags" is needed for any
> non-trivial network I/O.
We have flags for sqes, depending on the type. You can add to the
union that already holds rw_flags/fsync_flags/poll_events? There's
also a (smaller) flags field that applies for all types, which
currently only holds the fixed file flag.
If you're talking on a per-syscall type of flag, io_uring_enter(2)
does take a flags member.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ 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