Jan Karcher wrote:
>
>
> On 26/08/2024 04:56, D. Wythe wrote:
> >
> >
> > On 8/22/24 3:19 PM, Jan Karcher wrote:
> >>
> >>
> >> On 21/08/2024 13:06, Jeongjun Park wrote:
> >>> Jan Karcher wrote:
> >>>>
> >>>>
> >>
> >> [...]
> >>
> >>>>
> >>>> 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.
> >>>
> >>> This is the patch I tested. Except for smc.h and smc_inet.c, the rest is
> >>> just a patch that changes smc->sk to smc->inet.sk. When I tested using
> >>> this patch and c repro, the vulnerability was not triggered.
> >>>
> >>> Regards,
> >>> Jeongjun Park
> >>
> >> Thank you for providing your changes. TBH, I do like only having the
> >> inet socket in our structure.
> >> I did not review it completley since there are, obviously, a lot of
> >> changes.
> >> Testing looks good so far but needs some more time.
> >>
> >> @D. Wythe are there any concerns from your side regarding this solution?
> >>
> >> Thanks,
> >> Jan
> >>
> >
> > Well, I really don't think this is a good idea. As we've mentioned, for
> > AF_SMC, smc_sock should not be treated as inet_sock.
> > While in terms of actual running logic, this approach yields the same
> > result as using a union, but the use of a union clearly indicates
> > that it includes two distinct types of socks.
>
> Fair. I understand both sides here and i do not have a strong opinion.
> One is kinda implicit, the other defines fields we do not use...
> Of course there would be a compromise to define another struct something
> like this:
>
> struct smc_sock_types {
> struct sock sk;
> #if IS_ENABLED(CONFIG_IPV6)
> struct ipv6_pinfo *pinet6;
> #endif
> };
>
> struct smc_sock { /* smc sock container */
> struct smc_sock_types socks;
> [...]
If absolutely must use the sock structure in smc_sock, I think it would
be okay to modify it like the patch below to avoid a lot of code m
odifications.
---
net/smc/smc.h | 3 +++
net/smc/smc_inet.c | 8 +++++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 34b781e463c4..ad77d6b6b8d3 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -284,6 +284,9 @@ struct smc_connection {
struct smc_sock { /* smc sock container */
struct sock sk;
+#if IS_ENABLED(CONFIG_IPV6)
+ struct ipv6_pinfo *pinet6;
+#endif
struct socket *clcsock; /* internal tcp socket */
void (*clcsk_state_change)(struct sock *sk);
/* original stat_change fct. */
diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
index bece346dd8e9..a5b2041600f9 100644
--- a/net/smc/smc_inet.c
+++ b/net/smc/smc_inet.c
@@ -60,6 +60,11 @@ static struct inet_protosw smc_inet_protosw = {
};
#if IS_ENABLED(CONFIG_IPV6)
+struct smc6_sock {
+ struct smc_sock smc;
+ struct ipv6_pinfo inet6;
+};
+
static struct proto smc_inet6_prot = {
.name = "INET6_SMC",
.owner = THIS_MODULE,
@@ -67,9 +72,10 @@ static struct proto smc_inet6_prot = {
.hash = smc_hash_sk,
.unhash = smc_unhash_sk,
.release_cb = smc_release_cb,
- .obj_size = sizeof(struct smc_sock),
+ .obj_size = sizeof(struct smc6_sock),
.h.smc_hash = &smc_v6_hashinfo,
.slab_flags = SLAB_TYPESAFE_BY_RCU,
+ .ipv6_pinfo_offset = offsetof(struct smc6_sock, inet6),
};
static const struct proto_ops smc_inet6_stream_ops = {
--
Regards,
Jeongjun Park
>
> That said, don't know if i like this either.
>
> Thanks
> - Jan
>
> >
> > Also, if you have to make this change, perhaps you can give it a try
> >
> > #define smc->sk smc->inet.sk
> >
> > This will save lots of modifications.
> >
> > Thanks,
> > D. Wythe
> >
> >>>
> >>>>
> >>>> The statement that SMC would be more aligned with other AFs is
> >>>> already a
> >>>> big win in my book.
> >>>>
> >>>> Thanks
> >>>> - Jan
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Paolo
> >>>>>
> >>>
