[PATCH mptcp-next v3 0/5] Squash to "Add mptcp_subflow bpf_iter support"

[PATCH mptcp-next v3 0/5] Squash to "Add mptcp_subflow bpf_iter support"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v3:
 - patch 3, continue to use sock_owned_by_user_nocheck() and spin_is_locked()
checks instead of using msk_owned_by_me().
 - patch 5, drop declaration of bpf_mptcp_subflow_tcp_sock. It's no longer
used.
 - patch 5, update the comment for mptcp_subflow_tcp_sock(), which is a BPF
helper, not a kfunc.

The commit log of "bpf: Register mptcp common kfunc set" doesn't match the
code, please update it as:

'''
bpf: Register mptcp common kfunc set

MPTCP helper mptcp_subflow_ctx() is used to convert struct sock to
struct mptcp_subflow_context. It will be used in MPTCP BPF programs.

This patch defines corresponding wrapper of this helper, and put it
into the newly defined mptcp common kfunc set and register this set
with the flag BPF_PROG_TYPE_CGROUP_SOCKOPT to let it accessible to
the 'cgroup/getsockopt' type of BPF programs.
'''

v2:
 - Drop bpf_skc_to_mptcp_sock
 - Check the owner before assigning the msk as Mat suggested.
 - Use bpf_core_cast() in mptcp_subflow bpf_iter subtest instead of
using bpf_skc_to_mptcp_sock().

Address Martin's suggestions for "Add mptcp_subflow bpf_iter support" v2.

Geliang Tang (5):
  Revert "bpf: Extend bpf_skc_to_mptcp_sock to MPTCP sock"
  Revert "bpf: Allow use of skc_to_mptcp_sock in cg_sockopt"
  Squash to "bpf: Add mptcp_subflow bpf_iter"
  Revert "bpf: Acquire and release mptcp socket"
  Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest"

 include/net/mptcp.h                           |  4 +-
 kernel/bpf/cgroup.c                           |  2 -
 net/core/filter.c                             |  2 +-
 net/mptcp/bpf.c                               | 41 +++++--------------
 .../testing/selftests/bpf/bpf_experimental.h  |  2 +-
 tools/testing/selftests/bpf/progs/mptcp_bpf.h |  5 ---
 .../selftests/bpf/progs/mptcp_bpf_iters.c     | 10 ++---
 7 files changed, 17 insertions(+), 49 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v3 1/5] Revert "bpf: Extend bpf_skc_to_mptcp_sock to MPTCP sock"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

bpf_core_cast() can be used in mptcp_subflow bpf_iter selftests to get the
msk, instead of using bpf_skc_to_mptcp_sock(). No need to add this patch
anymore, revert it.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h |  4 ++--
 net/core/filter.c   |  2 +-
 net/mptcp/bpf.c     | 10 ++--------
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 72d6e6597add..2c85ca92bb1c 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -323,9 +323,9 @@ static inline void mptcpv6_handle_mapped(struct sock *sk, bool mapped) { }
 #endif
 
 #if defined(CONFIG_MPTCP) && defined(CONFIG_BPF_SYSCALL)
-struct mptcp_sock *bpf_mptcp_sock_from_sock(struct sock *sk);
+struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk);
 #else
-static inline struct mptcp_sock *bpf_mptcp_sock_from_sock(struct sock *sk) { return NULL; }
+static inline struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk) { return NULL; }
 #endif
 
 #if !IS_ENABLED(CONFIG_MPTCP)
diff --git a/net/core/filter.c b/net/core/filter.c
index fc094e8d53d3..2ec162dd83c4 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11843,7 +11843,7 @@ const struct bpf_func_proto bpf_skc_to_unix_sock_proto = {
 BPF_CALL_1(bpf_skc_to_mptcp_sock, struct sock *, sk)
 {
 	BTF_TYPE_EMIT(struct mptcp_sock);
-	return (unsigned long)bpf_mptcp_sock_from_sock(sk);
+	return (unsigned long)bpf_mptcp_sock_from_subflow(sk);
 }
 
 const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index be222fa5f308..7e9d9c9a04cf 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -195,15 +195,9 @@ static struct bpf_struct_ops bpf_mptcp_sched_ops = {
 };
 #endif /* CONFIG_BPF_JIT */
 
-struct mptcp_sock *bpf_mptcp_sock_from_sock(struct sock *sk)
+struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
 {
-	if (unlikely(!sk || !sk_fullsock(sk)))
-		return NULL;
-
-	if (sk->sk_protocol == IPPROTO_MPTCP)
-		return mptcp_sk(sk);
-
-	if (sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
+	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
 		return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
 
 	return NULL;
-- 
2.43.0

[PATCH mptcp-next v3 2/5] Revert "bpf: Allow use of skc_to_mptcp_sock in cg_sockopt"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

bpf_core_cast() can be used in mptcp_subflow bpf_iter selftests to get the
msk, instead of using bpf_skc_to_mptcp_sock(). No need to add this patch
anymore, revert it.

This reverts commit d8d42f5a4542c2dc31e4e00c58508a821c6ab789.
---
 kernel/bpf/cgroup.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 1ca22e4842cf..46e5db65dbc8 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -2358,8 +2358,6 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 #ifdef CONFIG_INET
 	case BPF_FUNC_tcp_sock:
 		return &bpf_tcp_sock_proto;
-	case BPF_FUNC_skc_to_mptcp_sock:
-		return &bpf_skc_to_mptcp_sock_proto;
 #endif
 	case BPF_FUNC_perf_event_output:
 		return &bpf_event_output_data_proto;
-- 
2.43.0

[PATCH mptcp-next v3 3/5] Squash to "bpf: Add mptcp_subflow bpf_iter"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Drop the NULL check for 'msk' as Martin suggested, add more checks
for 'sk'.

Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as the
argument in the bpf_iter_mptcp_subflow_new as Martin suggested.

v3:
 - continue to use sock_owned_by_user_nocheck and spin_is_locked
checks instead of using msk_owned_by_me().

v2:
 - check the owner before assigning the msk as Mat suggested.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/bpf.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 7e9d9c9a04cf..06fb0f88e35f 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -235,24 +235,28 @@ bpf_mptcp_subflow_ctx(const struct sock *sk)
 
 __bpf_kfunc static int
 bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
-			   struct mptcp_sock *msk)
+			   struct sock *sk)
 {
 	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
-	struct sock *sk = (struct sock *)msk;
+	struct mptcp_sock *msk;
 
 	BUILD_BUG_ON(sizeof(struct bpf_iter_mptcp_subflow_kern) >
 		     sizeof(struct bpf_iter_mptcp_subflow));
 	BUILD_BUG_ON(__alignof__(struct bpf_iter_mptcp_subflow_kern) !=
 		     __alignof__(struct bpf_iter_mptcp_subflow));
 
-	kit->msk = msk;
-	if (!msk)
+	if (unlikely(!sk || !sk_fullsock(sk)))
+		return -EINVAL;
+
+	if (sk->sk_protocol != IPPROTO_MPTCP)
 		return -EINVAL;
 
 	if (!sock_owned_by_user_nocheck(sk) &&
 	    !spin_is_locked(&sk->sk_lock.slock))
 		return -EINVAL;
 
+	msk = mptcp_sk(sk);
+	kit->msk = msk;
 	kit->pos = &msk->conn_list;
 	return 0;
 }
-- 
2.43.0

[PATCH mptcp-next v3 4/5] Revert "bpf: Acquire and release mptcp socket"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Drop this patch as Martin suggested.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/bpf.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 06fb0f88e35f..2256d4de1e20 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -278,23 +278,6 @@ bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it)
 {
 }
 
-__bpf_kfunc static struct
-mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk)
-{
-	struct sock *sk = (struct sock *)msk;
-
-	if (sk && refcount_inc_not_zero(&sk->sk_refcnt))
-		return msk;
-	return NULL;
-}
-
-__bpf_kfunc static void bpf_mptcp_sock_release(struct mptcp_sock *msk)
-{
-	struct sock *sk = (struct sock *)msk;
-
-	WARN_ON_ONCE(!sk || !refcount_dec_not_one(&sk->sk_refcnt));
-}
-
 __bpf_kfunc struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos)
 {
@@ -315,8 +298,6 @@ BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx, KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new, KF_ITER_NEW | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next, KF_ITER_NEXT | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy, KF_ITER_DESTROY)
-BTF_ID_FLAGS(func, bpf_mptcp_sock_acquire, KF_ACQUIRE | KF_RET_NULL)
-BTF_ID_FLAGS(func, bpf_mptcp_sock_release, KF_RELEASE)
 BTF_KFUNCS_END(bpf_mptcp_common_kfunc_ids)
 
 static const struct btf_kfunc_id_set bpf_mptcp_common_kfunc_set = {
-- 
2.43.0

[PATCH mptcp-next v3 5/5] Squash to "selftests/bpf: Add mptcp_subflow bpf_iter subtest"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Use bpf_core_cast() instead of bpf_skc_to_mptcp_sock().
Change the 2nd parameter type of bpf_for_each() as 'struct sock'.
Drop use of bpf_mptcp_sock_acquire/release.
Drop declaration of bpf_mptcp_subflow_tcp_sock. It's no longer used.
Update the comment for mptcp_subflow_tcp_sock(), which is a BPF helper,
not a kfunc.

Please update the commit log as:

'''
This patch adds a "cgroup/getsockopt" program "iters_subflow" to test the
newly added mptcp_subflow bpf_iter.

Export mptcp_subflow helpers bpf_iter_mptcp_subflow_new/_next/_destroy
and other helpers into bpf_experimental.h.

Use bpf_for_each() to walk the subflow list of this msk. MPTCP-specific
packet scheduler kfunc can be called in the loop. In this test, just
add all subflow ids to local variable local_ids, then invoke the helper
mptcp_subflow_tcp_sock() in the loop to pick a subsocket.

Out of the loop, use bpf_mptcp_subflow_ctx() to get the subflow context
of the picked subsocket and do some verification. Finally, assign
local_ids to global variable ids so that the application can obtain this
value.

Add a subtest named test_iters_subflow to load and verify the newly added
mptcp_subflow type bpf_iter example in test_mptcp. Use the helper
endpoint_init() to add 3 new subflow endpoints. Send a byte of message
to start the mptcp connection, and wait for new subflows to be added.
getsockopt() is invoked to trigger the "cgroup/getsockopt" test program
"iters_subflow". Check if skel->bss->ids equals 10 to verify whether this
mptcp_subflow bpf_iter loops correctly as expected.
'''

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/bpf_experimental.h      |  2 +-
 tools/testing/selftests/bpf/progs/mptcp_bpf.h       |  5 -----
 tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c | 10 +++-------
 3 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index 2ab3f0063c0f..6a96c56f0725 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -577,7 +577,7 @@ extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak __ksym;
 
 struct bpf_iter_mptcp_subflow;
 extern int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
-				      struct mptcp_sock *msk) __weak __ksym;
+				      struct sock *sk) __weak __ksym;
 extern struct mptcp_subflow_context *
 bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
 extern void
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
index b1f6e1fb467e..72263672510a 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
@@ -43,13 +43,8 @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
 }
 
 /* ksym */
-extern struct mptcp_sock *bpf_mptcp_sock_acquire(struct mptcp_sock *msk) __ksym;
-extern void bpf_mptcp_sock_release(struct mptcp_sock *msk) __ksym;
-
 extern struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
-extern struct sock *
-bpf_mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow) __ksym;
 
 extern void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
 					bool scheduled) __ksym;
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
index fd5691a4073b..a1d8f9b20259 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
@@ -24,14 +24,11 @@ int iters_subflow(struct bpf_sockopt *ctx)
 	if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
 		return 1;
 
-	msk = bpf_skc_to_mptcp_sock(sk);
+	msk = bpf_core_cast(sk, struct mptcp_sock);
 	if (!msk || msk->pm.server_side || !msk->pm.subflows)
 		return 1;
 
-	msk = bpf_mptcp_sock_acquire(msk);
-	if (!msk)
-		return 1;
-	bpf_for_each(mptcp_subflow, subflow, msk) {
+	bpf_for_each(mptcp_subflow, subflow, (struct sock *)sk) {
 		/* Here MPTCP-specific packet scheduler kfunc can be called:
 		 * this test is not doing anything really useful, only to
 		 * verify the iteration works.
@@ -39,7 +36,7 @@ int iters_subflow(struct bpf_sockopt *ctx)
 
 		local_ids += subflow->subflow_id;
 
-		/* only to check the following kfunc works */
+		/* only to check the following helper works */
 		ssk = mptcp_subflow_tcp_sock(subflow);
 	}
 
@@ -58,6 +55,5 @@ int iters_subflow(struct bpf_sockopt *ctx)
 	ids = local_ids;
 
 out:
-	bpf_mptcp_sock_release(msk);
 	return 1;
 }
-- 
2.43.0

Re: [PATCH mptcp-next v3 0/5] Squash to "Add mptcp_subflow bpf_iter support"

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13368589515

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/c163f5827a44
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=934633


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next v3 3/5] Squash to "bpf: Add mptcp_subflow bpf_iter"

[Matthieu Baerts]

Hi Geliang,

On 17/02/2025 11:34, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Drop the NULL check for 'msk' as Martin suggested, add more checks
> for 'sk'.
> 
> Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as the
> argument in the bpf_iter_mptcp_subflow_new as Martin suggested.
> 
> v3:
>  - continue to use sock_owned_by_user_nocheck and spin_is_locked
> checks instead of using msk_owned_by_me().

Why should we continue to do so?

From what I understood, with the current version, this new
bpf_iter_mptcp_subflow_new kfunc can only be called from a cg sockopt
bpf prog, which means these checks are not needed for the moment, no?

So maybe we should only add these checks later, when extending its usage
to struct_ops? In the meantime, msk_owned_by_me() could then be used
instead?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v3 3/5] Squash to "bpf: Add mptcp_subflow bpf_iter"

[Geliang Tang]

Hi Mat,

Thanks for the review.

On Thu, 2025-02-20 at 19:25 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 17/02/2025 11:34, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Drop the NULL check for 'msk' as Martin suggested, add more checks
> > for 'sk'.
> > 
> > Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as
> > the
> > argument in the bpf_iter_mptcp_subflow_new as Martin suggested.
> > 
> > v3:
> >  - continue to use sock_owned_by_user_nocheck and spin_is_locked
> > checks instead of using msk_owned_by_me().
> 
> Why should we continue to do so?
> 
> From what I understood, with the current version, this new
> bpf_iter_mptcp_subflow_new kfunc can only be called from a cg sockopt
> bpf prog, which means these checks are not needed for the moment, no?

Thanks for the clarification. I re-read Martin's comments and he also
suggested removing these checks. So I removed it in v4.

> 
> So maybe we should only add these checks later, when extending its
> usage
> to struct_ops? In the meantime, msk_owned_by_me() could then be used
> instead?

Do I need to add a patch into the "use bpf_iter in bpf schedulers" set
that is under review to do these checks? Do you mean that using
msk_owned_by_me() is enough for struct_ops, and there is no need to use
sock_owned_by_user_nocheck() and spin_is_locked() checks?

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v3 3/5] Squash to "bpf: Add mptcp_subflow bpf_iter"

[Matthieu Baerts]

Hi Geliang,

On 24/02/2025 09:31, Geliang Tang wrote:
> Hi Mat,
> 
> Thanks for the review.
> 
> On Thu, 2025-02-20 at 19:25 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 17/02/2025 11:34, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> Drop the NULL check for 'msk' as Martin suggested, add more checks
>>> for 'sk'.
>>>
>>> Use the "struct sock *sk" instead of "struct mptcp-sock *msk" as
>>> the
>>> argument in the bpf_iter_mptcp_subflow_new as Martin suggested.
>>>
>>> v3:
>>>  - continue to use sock_owned_by_user_nocheck and spin_is_locked
>>> checks instead of using msk_owned_by_me().
>>
>> Why should we continue to do so?
>>
>> From what I understood, with the current version, this new
>> bpf_iter_mptcp_subflow_new kfunc can only be called from a cg sockopt
>> bpf prog, which means these checks are not needed for the moment, no?
> 
> Thanks for the clarification. I re-read Martin's comments and he also
> suggested removing these checks. So I removed it in v4.

Thanks!

>> So maybe we should only add these checks later, when extending its
>> usage
>> to struct_ops? In the meantime, msk_owned_by_me() could then be used
>> instead?
> 
> Do I need to add a patch into the "use bpf_iter in bpf schedulers" set
> that is under review to do these checks? Do you mean that using
> msk_owned_by_me() is enough for struct_ops, and there is no need to use
> sock_owned_by_user_nocheck() and spin_is_locked() checks?

My understanding is that if a kfunc is set to used in 'struct_ops', it
can be used with any 'struct_ops', and not a specific one. Is this correct?

- If yes, it means we don't really know in which context a kfunc will be
used, thus we need additional checks. But... this feels wrong: if
kfunc's cannot be restricted to specific 'struct_ops', probably this
should be reported to the BPF maintainers, and see if it could be
possible to have such restriction not to add unneeded checks at run time.

- If the MPTCP kfunc can be restricted to MPTCP 'struct_ops' where we
know the msk lock will be taken, then no need to add these checks if
they are useless.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.