| Message ID | 20240228074308.3623984-2-zegao@tencent.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] net, sock.h: Make sure accesses to a fullsock when it is indeed one | expand |
On Wed, Feb 28, 2024 at 8:46 AM Ze Gao <zegao2021@gmail.com> wrote: > > We know a pointer that has type struct sock* can actually points to > one of some different sock types which have different memory layouts, > take req_to_sk() for example, and whether a sock is full or not > depends upon ->sk_state which is a shared field among them so that we > see some repeated code pattern similar to this: > > if (sk && sk fullsock(sk) && sk->field_not_shared) > > which seems to have no problem at the first glance, but it is actually > unsound in a way that ->field_not_shared is likely uninitialized (or > unmapped) when it's not a full sock, and a compiler is free to reorder > accesses to fields of a struct sock when it can, that is, it could > reorder accesses to ->field_not_shared across ->sk_state or load them > all before the branch test, which leads to unexpected behavior, although > most of them won't do this. > > So leave a barrier() in between and force the compiler to keep the > obvious program order. > > Cc: Honglin Li <honglinli@tencent.com> > Signed-off-by: Ze Gao <zegao@tencent.com> > --- > > IIUC, casting a pointer to refer to a bigger object in size is > technically UB, which may lead to unsound code. From the POV of > a compiler, when it is allowed to assume that one struct member > is valid, they all are through a pointer, and thus it's likely > for the compiler to do such optimizations and reorder what we > want to keep in order. > > Note this is not a typical way to use barrier(), which only > acts an ok fix to what's already unsound, at least IMO. > > Comments are welcome, since I'm not an expert in C and I know > most of compilers won't do this reorder, but I'm being pessimistic > here. Well, my suggestion is to have evidence first... We are not going to add barriers just because we do not trust compilers handling of sequence points.
On Wed, Feb 28, 2024 at 4:34 PM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Feb 28, 2024 at 8:46 AM Ze Gao <zegao2021@gmail.com> wrote: > > > > We know a pointer that has type struct sock* can actually points to > > one of some different sock types which have different memory layouts, > > take req_to_sk() for example, and whether a sock is full or not > > depends upon ->sk_state which is a shared field among them so that we > > see some repeated code pattern similar to this: > > > > if (sk && sk fullsock(sk) && sk->field_not_shared) > > > > which seems to have no problem at the first glance, but it is actually > > unsound in a way that ->field_not_shared is likely uninitialized (or > > unmapped) when it's not a full sock, and a compiler is free to reorder > > accesses to fields of a struct sock when it can, that is, it could > > reorder accesses to ->field_not_shared across ->sk_state or load them > > all before the branch test, which leads to unexpected behavior, although > > most of them won't do this. > > > > So leave a barrier() in between and force the compiler to keep the > > obvious program order. > > > > Cc: Honglin Li <honglinli@tencent.com> > > Signed-off-by: Ze Gao <zegao@tencent.com> > > --- > > > > IIUC, casting a pointer to refer to a bigger object in size is > > technically UB, which may lead to unsound code. From the POV of > > a compiler, when it is allowed to assume that one struct member > > is valid, they all are through a pointer, and thus it's likely > > for the compiler to do such optimizations and reorder what we > > want to keep in order. > > > > Note this is not a typical way to use barrier(), which only > > acts an ok fix to what's already unsound, at least IMO. > > > > Comments are welcome, since I'm not an expert in C and I know > > most of compilers won't do this reorder, but I'm being pessimistic > > here. > > Well, my suggestion is to have evidence first... Fair point! my initial purpose is to raise my question here to check if there is UB here and see if C experts have insights on this. > We are not going to add barriers just because we do not trust > compilers handling of sequence points. Makes sense to me as well if this is indeed not an issue here. Thanks, -- Ze
diff --git a/include/net/sock.h b/include/net/sock.h index 92f7ea62a915..f7e3960cb5fc 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -2815,7 +2815,14 @@ skb_sk_is_prefetched(struct sk_buff *skb) */ static inline bool sk_fullsock(const struct sock *sk) { - return (1 << sk->sk_state) & ~(TCPF_TIME_WAIT | TCPF_NEW_SYN_RECV); + bool ret = (1 << sk->sk_state) & ~(TCPF_TIME_WAIT | TCPF_NEW_SYN_RECV); + + /* + * Make sure all accesses to a full sock happens right + * after ->sk_state. + */ + barrier(); + return ret; } static inline bool
We know a pointer that has type struct sock* can actually points to one of some different sock types which have different memory layouts, take req_to_sk() for example, and whether a sock is full or not depends upon ->sk_state which is a shared field among them so that we see some repeated code pattern similar to this: if (sk && sk fullsock(sk) && sk->field_not_shared) which seems to have no problem at the first glance, but it is actually unsound in a way that ->field_not_shared is likely uninitialized (or unmapped) when it's not a full sock, and a compiler is free to reorder accesses to fields of a struct sock when it can, that is, it could reorder accesses to ->field_not_shared across ->sk_state or load them all before the branch test, which leads to unexpected behavior, although most of them won't do this. So leave a barrier() in between and force the compiler to keep the obvious program order. Cc: Honglin Li <honglinli@tencent.com> Signed-off-by: Ze Gao <zegao@tencent.com> --- IIUC, casting a pointer to refer to a bigger object in size is technically UB, which may lead to unsound code. From the POV of a compiler, when it is allowed to assume that one struct member is valid, they all are through a pointer, and thus it's likely for the compiler to do such optimizations and reorder what we want to keep in order. Note this is not a typical way to use barrier(), which only acts an ok fix to what's already unsound, at least IMO. Comments are welcome, since I'm not an expert in C and I know most of compilers won't do this reorder, but I'm being pessimistic here. Happy to learn from your sage insights and better solutions (or no solutions at all if this is indeed not a problem in the first place) Regards, -- Ze include/net/sock.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)