Users of ipcm6_init() have a somewhat complex post initialisation
of ->dontfrag and ->tclass. Not only it adds additional overhead,
but also complicates the code.
First, replace ipcm6_init() with ipcm6_init_sk(). As it might be not an
equivalent change, let's first look at ->dontfrag. The logic was to set
it from cmsg if specified and otherwise fallback to np->dontfrag. Now
it's initialising to np->dontfrag in the beginning and then potentially
overriding with cmsg, which is absolutely the same behaviour.
It's a bit more complex with ->tclass as ip6_datagram_send_ctl() might
set it to -1, which is a default and not valid value. The solution
here is to skip -1's specified in cmsg, so it'll be left with the socket
default value getting us to the old behaviour.
Signed-off-by: Pavel Begunkov <[email protected]>
---
include/net/ipv6.h | 9 ---------
net/ipv6/datagram.c | 4 ++--
net/ipv6/ip6_output.c | 2 --
net/ipv6/raw.c | 8 +-------
net/ipv6/udp.c | 7 +------
net/l2tp/l2tp_ip6.c | 8 +-------
6 files changed, 5 insertions(+), 33 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 213612f1680c..30a3447e34b4 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -352,15 +352,6 @@ struct ipcm6_cookie {
struct ipv6_txoptions *opt;
};
-static inline void ipcm6_init(struct ipcm6_cookie *ipc6)
-{
- *ipc6 = (struct ipcm6_cookie) {
- .hlimit = -1,
- .tclass = -1,
- .dontfrag = -1,
- };
-}
-
static inline void ipcm6_init_sk(struct ipcm6_cookie *ipc6,
const struct ipv6_pinfo *np)
{
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 206f66310a88..1b334bc855ae 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -1003,9 +1003,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
if (tc < -1 || tc > 0xff)
goto exit_f;
+ if (tc != -1)
+ ipc6->tclass = tc;
err = 0;
- ipc6->tclass = tc;
-
break;
}
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 1f3d777e7694..976554d0fdec 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -2001,8 +2001,6 @@ struct sk_buff *ip6_make_skb(struct sock *sk,
ip6_cork_release(cork, &v6_cork);
return ERR_PTR(err);
}
- if (ipc6->dontfrag < 0)
- ipc6->dontfrag = inet6_sk(sk)->dontfrag;
err = __ip6_append_data(sk, &queue, cork, &v6_cork,
¤t->task_frag, getfrag, from,
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 0d7c13d33d1a..4582e432fa9f 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -808,7 +808,7 @@ static int rawv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
fl6.flowi6_mark = sk->sk_mark;
fl6.flowi6_uid = sk->sk_uid;
- ipcm6_init(&ipc6);
+ ipcm6_init_sk(&ipc6, np);
ipc6.sockc.tsflags = sk->sk_tsflags;
ipc6.sockc.mark = sk->sk_mark;
@@ -920,9 +920,6 @@ static int rawv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
if (hdrincl)
fl6.flowi6_flags |= FLOWI_FLAG_KNOWN_NH;
- if (ipc6.tclass < 0)
- ipc6.tclass = np->tclass;
-
fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
dst = ip6_dst_lookup_flow(sock_net(sk), sk, &fl6, final_p);
@@ -933,9 +930,6 @@ static int rawv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
if (ipc6.hlimit < 0)
ipc6.hlimit = ip6_sk_dst_hoplimit(np, &fl6, dst);
- if (ipc6.dontfrag < 0)
- ipc6.dontfrag = np->dontfrag;
-
if (msg->msg_flags&MSG_CONFIRM)
goto do_confirm;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index db9449b52dbe..de8382930910 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1313,7 +1313,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
int is_udplite = IS_UDPLITE(sk);
int (*getfrag)(void *, char *, int, int, int, struct sk_buff *);
- ipcm6_init(&ipc6);
+ ipcm6_init_sk(&ipc6, np);
ipc6.gso_size = READ_ONCE(up->gso_size);
ipc6.sockc.tsflags = sk->sk_tsflags;
ipc6.sockc.mark = sk->sk_mark;
@@ -1518,9 +1518,6 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
security_sk_classify_flow(sk, flowi6_to_flowi_common(fl6));
- if (ipc6.tclass < 0)
- ipc6.tclass = np->tclass;
-
fl6->flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6->flowlabel);
dst = ip6_sk_dst_lookup_flow(sk, fl6, final_p, connected);
@@ -1566,8 +1563,6 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
up->pending = AF_INET6;
do_append_data:
- if (ipc6.dontfrag < 0)
- ipc6.dontfrag = np->dontfrag;
up->len += ulen;
err = ip6_append_data(sk, getfrag, msg, ulen, sizeof(struct udphdr),
&ipc6, fl6, (struct rt6_info *)dst,
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 217c7192691e..12406789bb28 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -521,7 +521,7 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
fl6.flowi6_mark = sk->sk_mark;
fl6.flowi6_uid = sk->sk_uid;
- ipcm6_init(&ipc6);
+ ipcm6_init_sk(&ipc6, np);
if (lsa) {
if (addr_len < SIN6_LEN_RFC2133)
@@ -608,9 +608,6 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
security_sk_classify_flow(sk, flowi6_to_flowi_common(&fl6));
- if (ipc6.tclass < 0)
- ipc6.tclass = np->tclass;
-
fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
dst = ip6_dst_lookup_flow(sock_net(sk), sk, &fl6, final_p);
@@ -622,9 +619,6 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
if (ipc6.hlimit < 0)
ipc6.hlimit = ip6_sk_dst_hoplimit(np, &fl6, dst);
- if (ipc6.dontfrag < 0)
- ipc6.dontfrag = np->dontfrag;
-
if (msg->msg_flags & MSG_CONFIRM)
goto do_confirm;
--
2.36.0
On Thu, 2022-04-28 at 11:56 +0100, Pavel Begunkov wrote:
> Users of ipcm6_init() have a somewhat complex post initialisation
> of ->dontfrag and ->tclass. Not only it adds additional overhead,
> but also complicates the code.
>
> First, replace ipcm6_init() with ipcm6_init_sk(). As it might be not an
> equivalent change, let's first look at ->dontfrag. The logic was to set
> it from cmsg if specified and otherwise fallback to np->dontfrag. Now
> it's initialising to np->dontfrag in the beginning and then potentially
> overriding with cmsg, which is absolutely the same behaviour.
>
> It's a bit more complex with ->tclass as ip6_datagram_send_ctl() might
> set it to -1, which is a default and not valid value. The solution
> here is to skip -1's specified in cmsg, so it'll be left with the socket
> default value getting us to the old behaviour.
>
> Signed-off-by: Pavel Begunkov <[email protected]>
> ---
> include/net/ipv6.h | 9 ---------
> net/ipv6/datagram.c | 4 ++--
> net/ipv6/ip6_output.c | 2 --
> net/ipv6/raw.c | 8 +-------
> net/ipv6/udp.c | 7 +------
> net/l2tp/l2tp_ip6.c | 8 +-------
> 6 files changed, 5 insertions(+), 33 deletions(-)
>
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index 213612f1680c..30a3447e34b4 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -352,15 +352,6 @@ struct ipcm6_cookie {
> struct ipv6_txoptions *opt;
> };
>
> -static inline void ipcm6_init(struct ipcm6_cookie *ipc6)
> -{
> - *ipc6 = (struct ipcm6_cookie) {
> - .hlimit = -1,
> - .tclass = -1,
> - .dontfrag = -1,
> - };
> -}
> -
> static inline void ipcm6_init_sk(struct ipcm6_cookie *ipc6,
> const struct ipv6_pinfo *np)
> {
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index 206f66310a88..1b334bc855ae 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -1003,9 +1003,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
> if (tc < -1 || tc > 0xff)
> goto exit_f;
>
> + if (tc != -1)
> + ipc6->tclass = tc;
> err = 0;
> - ipc6->tclass = tc;
> -
> break;
> }
It looks like the above causes a behavioral change: before this patch
cmsg took precedence on socket status, after this patch looks like it's
the opposide.
Am I missing something?
Thanks
Paolo
On 4/28/22 15:04, Paolo Abeni wrote:
> On Thu, 2022-04-28 at 11:56 +0100, Pavel Begunkov wrote:
>> Users of ipcm6_init() have a somewhat complex post initialisation
>> of ->dontfrag and ->tclass. Not only it adds additional overhead,
>> but also complicates the code.
>>
>> First, replace ipcm6_init() with ipcm6_init_sk(). As it might be not an
>> equivalent change, let's first look at ->dontfrag. The logic was to set
>> it from cmsg if specified and otherwise fallback to np->dontfrag. Now
>> it's initialising to np->dontfrag in the beginning and then potentially
>> overriding with cmsg, which is absolutely the same behaviour.
>>
>> It's a bit more complex with ->tclass as ip6_datagram_send_ctl() might
>> set it to -1, which is a default and not valid value. The solution
>> here is to skip -1's specified in cmsg, so it'll be left with the socket
>> default value getting us to the old behaviour.
>>
>> Signed-off-by: Pavel Begunkov <[email protected]>
>> ---
>> include/net/ipv6.h | 9 ---------
>> net/ipv6/datagram.c | 4 ++--
>> net/ipv6/ip6_output.c | 2 --
>> net/ipv6/raw.c | 8 +-------
>> net/ipv6/udp.c | 7 +------
>> net/l2tp/l2tp_ip6.c | 8 +-------
>> 6 files changed, 5 insertions(+), 33 deletions(-)
>>
>> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
>> index 213612f1680c..30a3447e34b4 100644
>> --- a/include/net/ipv6.h
>> +++ b/include/net/ipv6.h
>> @@ -352,15 +352,6 @@ struct ipcm6_cookie {
>> struct ipv6_txoptions *opt;
>> };
>>
>> -static inline void ipcm6_init(struct ipcm6_cookie *ipc6)
>> -{
>> - *ipc6 = (struct ipcm6_cookie) {
>> - .hlimit = -1,
>> - .tclass = -1,
>> - .dontfrag = -1,
>> - };
>> -}
>> -
>> static inline void ipcm6_init_sk(struct ipcm6_cookie *ipc6,
>> const struct ipv6_pinfo *np)
>> {
>> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
>> index 206f66310a88..1b334bc855ae 100644
>> --- a/net/ipv6/datagram.c
>> +++ b/net/ipv6/datagram.c
>> @@ -1003,9 +1003,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
>> if (tc < -1 || tc > 0xff)
>> goto exit_f;
>>
>> + if (tc != -1)
>> + ipc6->tclass = tc;
>> err = 0;
>> - ipc6->tclass = tc;
>> -
>> break;
>> }
>
> It looks like the above causes a behavioral change: before this patch
> cmsg took precedence on socket status, after this patch looks like it's
> the opposide.
>
> Am I missing something?
before:
ipc6.tclass = -1;
if (cmsg)
ip6_datagram_send_ctl(&ipc6);
if (ipc6.tclass < 0)
ipc6.tclass = np->tclass;
after:
ipc6.tclass = np->tclass; // ipcm6_init_sk()
if (cmsg)
ip6_datagram_send_ctl(&ipc6);
Both should prioritise cmsg. The only catch is when tclass is
specified in cmsg but it's -1. The old version would assign
np->tclass in the end, the new one does the same but with
this added "if" in ip6_datagram_send_ctl() in the chunk
you quoted. Unless I missed something as well.
--
Pavel Begunkov