On 20/08/2024 12:45, Paolo Abeni wrote:
On 8/15/24 06:39, Jeongjun Park wrote:
Since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically
point to the same address, when smc_create_clcsk() stores the newly
created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted
into clcsock. This causes NULL pointer dereference and various other
memory corruptions.
To solve this, we need to modify the smc_sock structure.
Fixes: ac7138746e14 ("smc: establish new socket family")
Signed-off-by: Jeongjun Park <aha310510@xxxxxxxxx>
---
net/smc/smc.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 34b781e463c4..0d67a02a6ab1 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -283,7 +283,10 @@ struct smc_connection {
};
struct smc_sock { /* smc sock container */
- struct sock sk;
+ union {
+ struct sock sk;
+ struct inet_sock inet;
+ };
struct socket *clcsock; /* internal tcp socket */
void (*clcsk_state_change)(struct sock *sk);
/* original stat_change fct. */
As per the running discussion here:
https://lore.kernel.org/all/5ad4de6f-48d4-4d1b-b062-e1cd2e8b3600@xxxxxxxxxxxxx/#t
you should include at least a add a comment to the union, describing
which one is used in which case.
My personal preference would be directly replacing 'struct sk' with
'struct inet_sock inet;' and adjust all the smc->sk access accordingly,
likely via a new helper.
I understand that would be much more invasive, but would align with
other AF.
Hi all,
thanks for looking into this patch and providing your view on this topic.
My personal prefernce is clean. I want to have a clean SMC protocol, in
order to get it on a good path for future improvements.
The union, IMO, is not clean. It makes the problem less implicit but the
problem is still there.
Jeongjun, do i understand correct that you've tested the change proposed
by Alexandra with more changes required?
"""
Although this fix would require more code changes, we tested the bug and
confirmed that it was not triggered and the functionality was working
normally.
"""
https://lore.kernel.org/all/20240814150558.46178-1-aha310510@xxxxxxxxx/
If so would you mind adding a helper for this check as Paolo suggested
and send it?
This way we see which change is better for the future.
The statement that SMC would be more aligned with other AFs is already a
big win in my book.
Thanks
- Jan
Thanks,
Paolo
