* [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct
@ 2023-12-25 9:46 David Laight
2023-12-25 9:51 ` [PATCH net-next 1/4] net: Use sockptr_is_kernel() instead of testing is_kernel David Laight
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: David Laight @ 2023-12-25 9:46 UTC (permalink / raw
To: netdev@vger.kernel.org, David S . Miller, kuba@kernel.org
Cc: eric.dumazet@gmail.com, martin.lau@linux.dev, Alexei Starovoitov,
Stephen Hemminger, Jens Axboe, Daniel Borkmann, Andrii Nakryiko
The original commit for sockptr_t tried to use the pointer value
to determine whether a pointer was user or kernel.
This can't work on some architectures and was buggy on x86.
So the is_kernel discriminator was added after the union of pointers.
However this is still open to misuse and accidents.
Replace the union with a struct and remove the is_kernel member.
The user and kernel values are now in different places.
The structure size doesn't change - it was always padded out to 'two pointers'.
The only functional difference is that NULL pointers are always 'user'.
So dereferencing will (usually) fault in copy_from_user() rather than
panic if supplied as a kernel address.
Simple driver code that uses kernel sockets still works.
I've not tested bpf - but that should work unless it is breaking
the rules.
The first three patches just change the code to use the helpers
from sockptr.h.
The functional change is in the fourth patch.
David Laight (4):
Use sockptr_is_kernel() instead of testing is_kernel.
Use bpfptr_is_kernel() instead of checking the is_kernel member.
Use the sockptr_t helpers.
Change sockptr_t to be a struct of a kernel and user pointer.
include/linux/bpfptr.h | 10 ++++------
include/linux/sockptr.h | 15 +++++----------
kernel/bpf/bpf_iter.c | 2 +-
kernel/bpf/btf.c | 2 +-
kernel/bpf/syscall.c | 12 ++++++------
kernel/bpf/verifier.c | 10 +++++-----
net/ipv4/ip_sockglue.c | 2 +-
net/ipv6/ipv6_sockglue.c | 2 +-
net/socket.c | 2 +-
9 files changed, 25 insertions(+), 32 deletions(-)
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next 1/4] net: Use sockptr_is_kernel() instead of testing is_kernel
2023-12-25 9:46 [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct David Laight
@ 2023-12-25 9:51 ` David Laight
2023-12-25 9:55 ` [PATCH net-next 2/4] bpf: Use bpfptr_is_kernel() instead of checking the is_kernel member David Laight
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2023-12-25 9:51 UTC (permalink / raw
To: 'netdev@vger.kernel.org', 'David S . Miller',
'kuba@kernel.org'
Cc: 'eric.dumazet@gmail.com', 'martin.lau@linux.dev',
'Alexei Starovoitov', 'Stephen Hemminger',
'Jens Axboe', 'Daniel Borkmann',
'Andrii Nakryiko'
Some changes to option handling directly accesses optval.is_kernel.
Use the sockptr_is_kernel() helper instead.
No functional change.
Signed-off-by: David Laight <david.laight@aculab.com>
---
net/ipv4/ip_sockglue.c | 2 +-
net/ipv6/ipv6_sockglue.c | 2 +-
net/socket.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 2efc53526a38..94b2f8c095f5 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -1647,7 +1647,7 @@ int do_ip_getsockopt(struct sock *sk, int level, int optname,
if (sk->sk_type != SOCK_STREAM)
return -ENOPROTOOPT;
- if (optval.is_kernel) {
+ if (sockptr_is_kernel(optval)) {
msg.msg_control_is_user = false;
msg.msg_control = optval.kernel;
} else {
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 7d661735cb9d..64fc52d928c1 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -1144,7 +1144,7 @@ int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
if (sk->sk_type != SOCK_STREAM)
return -ENOPROTOOPT;
- if (optval.is_kernel) {
+ if (sockptr_is_kernel(optval)) {
msg.msg_control_is_user = false;
msg.msg_control = optval.kernel;
} else {
diff --git a/net/socket.c b/net/socket.c
index 3379c64217a4..8821f083ab0a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2366,7 +2366,7 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
} else if (unlikely(!ops->getsockopt)) {
err = -EOPNOTSUPP;
} else {
- if (WARN_ONCE(optval.is_kernel || optlen.is_kernel,
+ if (WARN_ONCE(sockptr_is_kernel(optval) || sockptr_is_kernel(optlen),
"Invalid argument type"))
return -EOPNOTSUPP;
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net-next 2/4] bpf: Use bpfptr_is_kernel() instead of checking the is_kernel member.
2023-12-25 9:46 [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct David Laight
2023-12-25 9:51 ` [PATCH net-next 1/4] net: Use sockptr_is_kernel() instead of testing is_kernel David Laight
@ 2023-12-25 9:55 ` David Laight
2023-12-25 9:57 ` [PATCH net-next 3/4] bpf: Use the sockptr_t helpers David Laight
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2023-12-25 9:55 UTC (permalink / raw
To: 'netdev@vger.kernel.org', 'David S . Miller',
'kuba@kernel.org'
Cc: 'eric.dumazet@gmail.com', 'martin.lau@linux.dev',
'Alexei Starovoitov', 'Stephen Hemminger',
'Jens Axboe', 'Daniel Borkmann',
'Andrii Nakryiko'
In some places the bpf code directly access the is_kernel member of bpfptr_t.
Change to use the bpfptr_is_kernel() helper.
No functional change.
Signed-off-by: David Laight <david.laight@aculab.com>
---
I'm not at all sure that the pattern:
urecord = make_bpfptr(attr->func_info, bpfptr_is_kernel(uattr));
isn't bending the rules somewhat - but that is a different issue.
kernel/bpf/bpf_iter.c | 2 +-
kernel/bpf/btf.c | 2 +-
kernel/bpf/syscall.c | 12 ++++++------
kernel/bpf/verifier.c | 10 +++++-----
4 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 0fae79164187..eb2c858dbf81 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -520,7 +520,7 @@ int bpf_iter_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
memset(&linfo, 0, sizeof(union bpf_iter_link_info));
- ulinfo = make_bpfptr(attr->link_create.iter_info, uattr.is_kernel);
+ ulinfo = make_bpfptr(attr->link_create.iter_info, bpfptr_is_kernel(uattr));
linfo_len = attr->link_create.iter_info_len;
if (bpfptr_is_null(ulinfo) ^ !linfo_len)
return -EINVAL;
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 15d71d2986d3..34720a1f586e 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5483,7 +5483,7 @@ static int finalize_log(struct bpf_verifier_log *log, bpfptr_t uattr, u32 uattr_
static struct btf *btf_parse(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
{
- bpfptr_t btf_data = make_bpfptr(attr->btf, uattr.is_kernel);
+ bpfptr_t btf_data = make_bpfptr(attr->btf, bpfptr_is_kernel(uattr));
char __user *log_ubuf = u64_to_user_ptr(attr->btf_log_buf);
struct btf_struct_metas *struct_meta_tab;
struct btf_verifier_env *env = NULL;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 0ed286b8a0f0..ba59fa8d02db 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -93,7 +93,7 @@ int bpf_check_uarg_tail_zero(bpfptr_t uaddr,
if (actual_size <= expected_size)
return 0;
- if (uaddr.is_kernel)
+ if (bpfptr_is_kernel(uaddr))
res = memchr_inv(uaddr.kernel + expected_size, 0,
actual_size - expected_size) == NULL;
else
@@ -1482,8 +1482,8 @@ static int map_lookup_elem(union bpf_attr *attr)
static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
{
- bpfptr_t ukey = make_bpfptr(attr->key, uattr.is_kernel);
- bpfptr_t uvalue = make_bpfptr(attr->value, uattr.is_kernel);
+ bpfptr_t ukey = make_bpfptr(attr->key, bpfptr_is_kernel(uattr));
+ bpfptr_t uvalue = make_bpfptr(attr->value, bpfptr_is_kernel(uattr));
int ufd = attr->map_fd;
struct bpf_map *map;
void *key, *value;
@@ -1538,7 +1538,7 @@ static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
static int map_delete_elem(union bpf_attr *attr, bpfptr_t uattr)
{
- bpfptr_t ukey = make_bpfptr(attr->key, uattr.is_kernel);
+ bpfptr_t ukey = make_bpfptr(attr->key, bpfptr_is_kernel(uattr));
int ufd = attr->map_fd;
struct bpf_map *map;
struct fd f;
@@ -2670,12 +2670,12 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
err = -EFAULT;
if (copy_from_bpfptr(prog->insns,
- make_bpfptr(attr->insns, uattr.is_kernel),
+ make_bpfptr(attr->insns, bpfptr_is_kernel(uattr)),
bpf_prog_insn_size(prog)) != 0)
goto free_prog_sec;
/* copy eBPF program license from user space */
if (strncpy_from_bpfptr(license,
- make_bpfptr(attr->license, uattr.is_kernel),
+ make_bpfptr(attr->license, bpfptr_is_kernel(uattr)),
sizeof(license) - 1) < 0)
goto free_prog_sec;
license[sizeof(license) - 1] = 0;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index af2819d5c8ee..42fea4966175 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -15838,7 +15838,7 @@ static int check_btf_func_early(struct bpf_verifier_env *env,
prog = env->prog;
btf = prog->aux->btf;
- urecord = make_bpfptr(attr->func_info, uattr.is_kernel);
+ urecord = make_bpfptr(attr->func_info, bpfptr_is_kernel(uattr));
min_size = min_t(u32, krec_size, urec_size);
krecord = kvcalloc(nfuncs, krec_size, GFP_KERNEL | __GFP_NOWARN);
@@ -15938,7 +15938,7 @@ static int check_btf_func(struct bpf_verifier_env *env,
prog = env->prog;
btf = prog->aux->btf;
- urecord = make_bpfptr(attr->func_info, uattr.is_kernel);
+ urecord = make_bpfptr(attr->func_info, bpfptr_is_kernel(uattr));
krecord = prog->aux->func_info;
info_aux = kcalloc(nfuncs, sizeof(*info_aux), GFP_KERNEL | __GFP_NOWARN);
@@ -16036,7 +16036,7 @@ static int check_btf_line(struct bpf_verifier_env *env,
s = 0;
sub = env->subprog_info;
- ulinfo = make_bpfptr(attr->line_info, uattr.is_kernel);
+ ulinfo = make_bpfptr(attr->line_info, bpfptr_is_kernel(uattr));
expected_size = sizeof(struct bpf_line_info);
ncopy = min_t(u32, expected_size, rec_size);
for (i = 0; i < nr_linfo; i++) {
@@ -16154,7 +16154,7 @@ static int check_core_relo(struct bpf_verifier_env *env,
rec_size % sizeof(u32))
return -EINVAL;
- u_core_relo = make_bpfptr(attr->core_relos, uattr.is_kernel);
+ u_core_relo = make_bpfptr(attr->core_relos, bpfptr_is_kernel(uattr));
expected_size = sizeof(struct bpf_core_relo);
ncopy = min_t(u32, expected_size, rec_size);
@@ -20790,7 +20790,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
env->insn_aux_data[i].orig_idx = i;
env->prog = *prog;
env->ops = bpf_verifier_ops[env->prog->type];
- env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
+ env->fd_array = make_bpfptr(attr->fd_array, bpfptr_is_kernel(uattr));
is_priv = bpf_capable();
bpf_get_btf_vmlinux();
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net-next 3/4] bpf: Use the sockptr_t helpers
2023-12-25 9:46 [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct David Laight
2023-12-25 9:51 ` [PATCH net-next 1/4] net: Use sockptr_is_kernel() instead of testing is_kernel David Laight
2023-12-25 9:55 ` [PATCH net-next 2/4] bpf: Use bpfptr_is_kernel() instead of checking the is_kernel member David Laight
@ 2023-12-25 9:57 ` David Laight
2023-12-25 9:58 ` [PATCH net-next 4/4] sockptr: Change sockptr_t to be a struct of a kernel and user pointer David Laight
2024-01-02 22:32 ` [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct Jakub Kicinski
4 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2023-12-25 9:57 UTC (permalink / raw
To: 'netdev@vger.kernel.org', 'David S . Miller',
'kuba@kernel.org'
Cc: 'eric.dumazet@gmail.com', 'martin.lau@linux.dev',
'Alexei Starovoitov', 'Stephen Hemminger',
'Jens Axboe', 'Daniel Borkmann',
'Andrii Nakryiko'
bpfptr_t is defined as sockptr_t but the bpfptr_is_kernel(),
bpfptr_is_null(), KERNEL_BPFPTR() and USER_BPFPTR() helpers are
copies of the sockptr ones.
Instead implement in terms of the sockptr helpers.
Signed-off-by: David Laight <david.laight@aculab.com>
---
include/linux/bpfptr.h | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/include/linux/bpfptr.h b/include/linux/bpfptr.h
index 79b2f78eec1a..862e87350477 100644
--- a/include/linux/bpfptr.h
+++ b/include/linux/bpfptr.h
@@ -10,17 +10,17 @@ typedef sockptr_t bpfptr_t;
static inline bool bpfptr_is_kernel(bpfptr_t bpfptr)
{
- return bpfptr.is_kernel;
+ return sockptr_is_kernel(bpfptr);
}
static inline bpfptr_t KERNEL_BPFPTR(void *p)
{
- return (bpfptr_t) { .kernel = p, .is_kernel = true };
+ return KERNEL_SOCKPTR(p);
}
static inline bpfptr_t USER_BPFPTR(void __user *p)
{
- return (bpfptr_t) { .user = p };
+ return USER_SOCKPTR(p);
}
static inline bpfptr_t make_bpfptr(u64 addr, bool is_kernel)
@@ -33,9 +33,7 @@ static inline bpfptr_t make_bpfptr(u64 addr, bool is_kernel)
static inline bool bpfptr_is_null(bpfptr_t bpfptr)
{
- if (bpfptr_is_kernel(bpfptr))
- return !bpfptr.kernel;
- return !bpfptr.user;
+ return sockptr_is_null(bpfptr);
}
static inline void bpfptr_add(bpfptr_t *bpfptr, size_t val)
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net-next 4/4] sockptr: Change sockptr_t to be a struct of a kernel and user pointer.
2023-12-25 9:46 [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct David Laight
` (2 preceding siblings ...)
2023-12-25 9:57 ` [PATCH net-next 3/4] bpf: Use the sockptr_t helpers David Laight
@ 2023-12-25 9:58 ` David Laight
2024-01-02 22:32 ` [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct Jakub Kicinski
4 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2023-12-25 9:58 UTC (permalink / raw
To: 'netdev@vger.kernel.org', 'David S . Miller',
'kuba@kernel.org'
Cc: 'eric.dumazet@gmail.com', 'martin.lau@linux.dev',
'Alexei Starovoitov', 'Stephen Hemminger',
'Jens Axboe', 'Daniel Borkmann',
'Andrii Nakryiko'
The original commit for sockptr_t tried to use the pointer value
to determine whether a pointer was user or kernel.
This can't work on some architecures and was buffy on x86.
So the is_kernel descriminator was added after the union of pointers.
However this is still open to misuse and accidents.
Replace the union with a struct and remove the is_kernel member.
The user and kernel values are now in different places.
The size doesn't change - it was always padded out to 'two pointers'.
The only functional difference is that NULL pointers are always 'user'.
So dereferncing will (usually) fault in copy_from_user() rather than
panic if supplied as a kernel address.
Simple driver code that uses kernel sockets still works.
I've not tested bpf - but that should work unless it is breaking
the rules.
Signed-off-by: David Laight <david.laight@aculab.com>
---
include/linux/sockptr.h | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h
index 307961b41541..7516c2ada6a8 100644
--- a/include/linux/sockptr.h
+++ b/include/linux/sockptr.h
@@ -12,21 +12,18 @@
#include <linux/uaccess.h>
typedef struct {
- union {
- void *kernel;
- void __user *user;
- };
- bool is_kernel : 1;
+ void *kernel;
+ void __user *user;
} sockptr_t;
static inline bool sockptr_is_kernel(sockptr_t sockptr)
{
- return sockptr.is_kernel;
+ return !!sockptr.kernel;
}
static inline sockptr_t KERNEL_SOCKPTR(void *p)
{
- return (sockptr_t) { .kernel = p, .is_kernel = true };
+ return (sockptr_t) { .kernel = p };
}
static inline sockptr_t USER_SOCKPTR(void __user *p)
@@ -36,9 +33,7 @@ static inline sockptr_t USER_SOCKPTR(void __user *p)
static inline bool sockptr_is_null(sockptr_t sockptr)
{
- if (sockptr_is_kernel(sockptr))
- return !sockptr.kernel;
- return !sockptr.user;
+ return !sockptr.user && !sockptr.kernel;
}
static inline int copy_from_sockptr_offset(void *dst, sockptr_t src,
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct
2023-12-25 9:46 [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct David Laight
` (3 preceding siblings ...)
2023-12-25 9:58 ` [PATCH net-next 4/4] sockptr: Change sockptr_t to be a struct of a kernel and user pointer David Laight
@ 2024-01-02 22:32 ` Jakub Kicinski
4 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2024-01-02 22:32 UTC (permalink / raw
To: David Laight
Cc: netdev@vger.kernel.org, David S . Miller, eric.dumazet@gmail.com,
martin.lau@linux.dev, Alexei Starovoitov, Stephen Hemminger,
Jens Axboe, Daniel Borkmann, Andrii Nakryiko
On Mon, 25 Dec 2023 09:46:23 +0000 David Laight wrote:
> The original commit for sockptr_t tried to use the pointer value
> to determine whether a pointer was user or kernel.
> This can't work on some architectures and was buggy on x86.
> So the is_kernel discriminator was added after the union of pointers.
>
> However this is still open to misuse and accidents.
> Replace the union with a struct and remove the is_kernel member.
> The user and kernel values are now in different places.
> The structure size doesn't change - it was always padded out to 'two pointers'.
>
> The only functional difference is that NULL pointers are always 'user'.
> So dereferencing will (usually) fault in copy_from_user() rather than
> panic if supplied as a kernel address.
>
> Simple driver code that uses kernel sockets still works.
> I've not tested bpf - but that should work unless it is breaking
> the rules.
LGTM, but we either need acks from bpf folks or route this via
bpf-next. So please repost and CC bpf@ on the whole series.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-02 22:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-25 9:46 [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct David Laight
2023-12-25 9:51 ` [PATCH net-next 1/4] net: Use sockptr_is_kernel() instead of testing is_kernel David Laight
2023-12-25 9:55 ` [PATCH net-next 2/4] bpf: Use bpfptr_is_kernel() instead of checking the is_kernel member David Laight
2023-12-25 9:57 ` [PATCH net-next 3/4] bpf: Use the sockptr_t helpers David Laight
2023-12-25 9:58 ` [PATCH net-next 4/4] sockptr: Change sockptr_t to be a struct of a kernel and user pointer David Laight
2024-01-02 22:32 ` [PATCH net-next 0/4] sockptr: Change sockptr_t to be a struct Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).