Coccicheck reports the error below:
net/mptcp/protocol.c:3330:15-28: ERROR: test of a variable/field address

Since the address of msk->cb_flags is used in __test_and_clear_bit, the
address should not be NULL. The judgment for if (unlikely(msk->cb_flags))
will always be true, we should check the real value of msk->cb_flags here.

Fixes: 65a569b03ca8 ("mptcp: optimize release_cb for the common case")
Signed-off-by: Xiang Yang <xiangyang3@huawei.com>
---
 net/mptcp/protocol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 65ee949a8a44..fae31dab49c9 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3327,7 +3327,7 @@ static void mptcp_release_cb(struct sock *sk)
 
 	if (__test_and_clear_bit(MPTCP_CLEAN_UNA, &msk->cb_flags))
 		__mptcp_clean_una_wakeup(sk);
-	if (unlikely(&msk->cb_flags)) {
+	if (unlikely(msk->cb_flags)) {
 		/* be sure to set the current sk state before tacking actions
 		 * depending on sk_state, that is processing MPTCP_ERROR_REPORT
 		 */
-- 
2.34.1

Hi Xiang Yang

On 03/08/2023 09:24, Xiang Yang wrote:
> Coccicheck reports the error below:
> net/mptcp/protocol.c:3330:15-28: ERROR: test of a variable/field address
> 
> Since the address of msk->cb_flags is used in __test_and_clear_bit, the
> address should not be NULL. The judgment for if (unlikely(msk->cb_flags))
> will always be true, we should check the real value of msk->cb_flags here.
> 
> Fixes: 65a569b03ca8 ("mptcp: optimize release_cb for the common case")
> Signed-off-by: Xiang Yang <xiangyang3@huawei.com>

This Coccicheck report was useful, the optimisation in place was not
working. But there was no impact apart from testing more conditions
where there were no reasons to.

The fix is then good to me but it should land in -net, not in net-next.

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

I don't know if it is needed to have a re-send just to change the subject.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

On Thu, 3 Aug 2023 18:32:15 +0200 Matthieu Baerts wrote:
> This Coccicheck report was useful, the optimisation in place was not
> working. But there was no impact apart from testing more conditions
> where there were no reasons to.
> 
> The fix is then good to me but it should land in -net, not in net-next.
> 
> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> 
> I don't know if it is needed to have a re-send just to change the subject.

Looks trivial enough to apply without a repost, but are you sure 
you don't want to take it into your tree? Run the selftests and all?

Hi Jakub,

3 Aug 2023 20:04:26 Jakub Kicinski <kuba@kernel.org>:

> On Thu, 3 Aug 2023 18:32:15 +0200 Matthieu Baerts wrote:
>> This Coccicheck report was useful, the optimisation in place was not
>> working. But there was no impact apart from testing more conditions
>> where there were no reasons to.
>>
>> The fix is then good to me but it should land in -net, not in net-next.
>>
>> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>
>> I don't know if it is needed to have a re-send just to change the subject.
>
> Looks trivial enough to apply without a repost, but are you sure
> you don't want to take it into your tree? Run the selftests and all?

Thank you for asking that! All patches sent to our mailing list are automatically tested but the report is only sent to our mailing list not to annoy too many people. This patch passed all tests we have:

https://lore.kernel.org/mptcp/20230803072438.1847500-1-xiangyang3@huawei.com/T/

I already applied it on our side. For non-trivial fixes or features, we usually prefer to keep them a bit only applied on our side for longer tests and to have syzkaller stressing them. But here, because this patch looks trivial enough, it seems fine to me to have it applied in -net directly.

Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

On Thu, 3 Aug 2023 22:18:18 +0200 (GMT+02:00) Matthieu Baerts wrote:
> Thank you for asking that! All patches sent to our mailing list are
> automatically tested but the report is only sent to our mailing list
> not to annoy too many people. This patch passed all tests we have:
> 
> https://lore.kernel.org/mptcp/20230803072438.1847500-1-xiangyang3@huawei.com/T/
> 
> I already applied it on our side. For non-trivial fixes or features,
> we usually prefer to keep them a bit only applied on our side for
> longer tests and to have syzkaller stressing them. But here, because
> this patch looks trivial enough, it seems fine to me to have it
> applied in -net directly.

GTK! I'll apply it in the evening, thanks!

Hi Xiang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4591732650672128
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4591732650672128/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6280582510936064
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6280582510936064/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5154682604093440
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5154682604093440/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5717632557514752
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5717632557514752/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/82019bf873cf


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-debug

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 (Tessares)