[PATCH net-next] net: tipc: fix FB_MTU eat two pages

From: Menglong Dong <redacted>

FB_MTU is used in 'tipc_msg_build()' to alloc smaller skb when memory
allocation fails, which can avoid unnecessary sending failures.

The value of FB_MTU now is 3744, and the data size will be:

  (3744 + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) + \
    SKB_DATA_ALIGN(BUF_HEADROOM + BUF_TAILROOM + 3))

which is larger than one page(4096), and two pages will be allocated.

To avoid it, replace '3744' with a calculation:

FB_MTU = (PAGE_SIZE - SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
          - SKB_DATA_ALIGN(BUF_HEADROOM + BUF_TAILROOM + 3))

Fixes: 4c94cc2d3d57 ("tipc: fall back to smaller MTU if allocation of local send skb fails")

Reported-by: Zeal Robot <redacted>
Signed-off-by: Menglong Dong <redacted>
---
 net/tipc/bcast.c |  1 +
 net/tipc/msg.c   |  8 +------
 net/tipc/msg.h   |  1 -
 net/tipc/mtu.h   | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 8 deletions(-)
 create mode 100644 net/tipc/mtu.h

diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index d4beca895992..c641b68e0812 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c

@@ -41,6 +41,7 @@
 #include "bcast.h"
 #include "link.h"
 #include "name_table.h"
+#include "mtu.h"
 
 #define BCLINK_WIN_DEFAULT  50	/* bcast link window size (default) */
 #define BCLINK_WIN_MIN      32	/* bcast minimum link window size */

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index ce6ab54822d8..ec70d271c2da 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c

@@ -40,15 +40,9 @@
 #include "addr.h"
 #include "name_table.h"
 #include "crypto.h"
+#include "mtu.h"
 
 #define MAX_FORWARD_SIZE 1024
-#ifdef CONFIG_TIPC_CRYPTO
-#define BUF_HEADROOM ALIGN(((LL_MAX_HEADER + 48) + EHDR_MAX_SIZE), 16)
-#define BUF_TAILROOM (TIPC_AES_GCM_TAG_SIZE)
-#else
-#define BUF_HEADROOM (LL_MAX_HEADER + 48)
-#define BUF_TAILROOM 16
-#endif
 
 static unsigned int align(unsigned int i)
 {

diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index 5d64596ba987..e83689d0f0f6 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h

@@ -99,7 +99,6 @@ struct plist;
 #define MAX_H_SIZE                60	/* Largest possible TIPC header size */
 
 #define MAX_MSG_SIZE (MAX_H_SIZE + TIPC_MAX_USER_MSG_SIZE)
-#define FB_MTU                  3744
 #define TIPC_MEDIA_INFO_OFFSET	5
 
 struct tipc_skb_cb {

diff --git a/net/tipc/mtu.h b/net/tipc/mtu.h
new file mode 100644
index 000000000000..033f0b178f9d
--- /dev/null
+++ b/net/tipc/mtu.h

@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2021 ZTE Corporation.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the names of the copyright holders nor the names of its
+ *    contributors may be used to endorse or promote products derived from
+ *    this software without specific prior written permission.
+ *
+ * Alternatively, this software may be distributed under the terms of the
+ * GNU General Public License ("GPL") version 2 as published by the Free
+ * Software Foundation.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _TIPC_MTU_H
+#define _TIPC_MTU_H
+
+#include <linux/tipc.h>
+#include "crypto.h"
+
+#ifdef CONFIG_TIPC_CRYPTO
+#define BUF_HEADROOM ALIGN(((LL_MAX_HEADER + 48) + EHDR_MAX_SIZE), 16)
+#define BUF_TAILROOM (TIPC_AES_GCM_TAG_SIZE)
+#define FB_MTU	(PAGE_SIZE - \
+		 SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \
+		 SKB_DATA_ALIGN(BUF_HEADROOM + BUF_TAILROOM + 3))
+#else
+#define BUF_HEADROOM (LL_MAX_HEADER + 48)
+#define BUF_TAILROOM 16
+#define FB_MTU	(PAGE_SIZE - \
+		 SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \
+		 SKB_DATA_ALIGN(BUF_HEADROOM + 3))
+#endif
+
+#endif

-- 
2.25.1

On 6/4/21 3:44 AM, menglong8.dong@gmail.com wrote:

Please don't add any extra file just for this little fix. We have enough 
files.
Keep the macros in msg.h/c where they used to be.  You can still add 
your copyright line to those files.
Regarding the macros kept inside msg.c, they are there because we design 
by the principle of minimal exposure, even among our module internal files.
Otherwise it is ok.

Thanks
///jon

Hello Maloy,

On Sat, Jun 5, 2021 at 3:20 AM Jon Maloy [off-list ref] wrote:

[...]

Please don't add any extra file just for this little fix. We have enough
files.
Keep the macros in msg.h/c where they used to be.  You can still add
your copyright line to those files.
Regarding the macros kept inside msg.c, they are there because we design
by the principle of minimal exposure, even among our module internal files.
Otherwise it is ok.

I don't want to add a new file too, but I found it's hard to define FB_MTU. I
tried to define it in msg.h, and 'crypto.h' should be included, which
'BUF_HEADROOM' is defined in. However, 'msg.h' is already included in
'crypto.h', so it doesn't work.

I tried to define FB_MTU in 'crypto.h', but it feels weird to define
it here. And
FB_MTU is also used in 'bcast.c', so it can't be defined in 'msg.c'.

I will see if there is a better solution.

Thanks!
Menglong Dong

quoted

@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2021 ZTE Corporation.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the names of the copyright holders nor the names of its
+ *    contributors may be used to endorse or promote products derived from
+ *    this software without specific prior written permission.
+ *
+ * Alternatively, this software may be distributed under the terms of the
+ * GNU General Public License ("GPL") version 2 as published by the Free
+ * Software Foundation.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _TIPC_MTU_H
+#define _TIPC_MTU_H
+
+#include <linux/tipc.h>
+#include "crypto.h"
+
+#ifdef CONFIG_TIPC_CRYPTO
+#define BUF_HEADROOM ALIGN(((LL_MAX_HEADER + 48) + EHDR_MAX_SIZE), 16)
+#define BUF_TAILROOM (TIPC_AES_GCM_TAG_SIZE)
+#define FB_MTU       (PAGE_SIZE - \
+              SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \
+              SKB_DATA_ALIGN(BUF_HEADROOM + BUF_TAILROOM + 3))
+#else
+#define BUF_HEADROOM (LL_MAX_HEADER + 48)
+#define BUF_TAILROOM 16
+#define FB_MTU       (PAGE_SIZE - \
+              SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \
+              SKB_DATA_ALIGN(BUF_HEADROOM + 3))
+#endif
+
+#endif

On 6/4/21 9:28 PM, Menglong Dong wrote:

Hello Maloy,

On Sat, Jun 5, 2021 at 3:20 AM Jon Maloy [off-list ref] wrote:

quoted

[...]

quoted

Please don't add any extra file just for this little fix. We have enough
files.
Keep the macros in msg.h/c where they used to be.  You can still add
your copyright line to those files.
Regarding the macros kept inside msg.c, they are there because we design
by the principle of minimal exposure, even among our module internal files.
Otherwise it is ok.

I don't want to add a new file too, but I found it's hard to define FB_MTU. I
tried to define it in msg.h, and 'crypto.h' should be included, which
'BUF_HEADROOM' is defined in. However, 'msg.h' is already included in
'crypto.h', so it doesn't work.

I tried to define FB_MTU in 'crypto.h', but it feels weird to define
it here. And
FB_MTU is also used in 'bcast.c', so it can't be defined in 'msg.c'.

I will see if there is a better solution.

I think we can leverage the fact that this by definition is a node local 
message, and those are never encrypted.
So, if you base FB_MTU on the non-crypto versions of BUF_HEADROOM and 
BUF_TAILROOM we should be safe.
That will even give us better utilization of the space available.

///jon

Thanks!
Menglong Dong

quoted

quoted

@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2021 ZTE Corporation.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the names of the copyright holders nor the names of its
+ *    contributors may be used to endorse or promote products derived from
+ *    this software without specific prior written permission.
+ *
+ * Alternatively, this software may be distributed under the terms of the
+ * GNU General Public License ("GPL") version 2 as published by the Free
+ * Software Foundation.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _TIPC_MTU_H
+#define _TIPC_MTU_H
+
+#include <linux/tipc.h>
+#include "crypto.h"
+
+#ifdef CONFIG_TIPC_CRYPTO
+#define BUF_HEADROOM ALIGN(((LL_MAX_HEADER + 48) + EHDR_MAX_SIZE), 16)
+#define BUF_TAILROOM (TIPC_AES_GCM_TAG_SIZE)
+#define FB_MTU       (PAGE_SIZE - \
+              SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \
+              SKB_DATA_ALIGN(BUF_HEADROOM + BUF_TAILROOM + 3))
+#else
+#define BUF_HEADROOM (LL_MAX_HEADER + 48)
+#define BUF_TAILROOM 16
+#define FB_MTU       (PAGE_SIZE - \
+              SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \
+              SKB_DATA_ALIGN(BUF_HEADROOM + 3))
+#endif
+
+#endif

On Sat, Jun 5, 2021 at 10:25 PM Jon Maloy [off-list ref] wrote:

On 6/4/21 9:28 PM, Menglong Dong wrote:

quoted

Hello Maloy,

On Sat, Jun 5, 2021 at 3:20 AM Jon Maloy [off-list ref] wrote:

quoted

[...]

quoted

Please don't add any extra file just for this little fix. We have enough
files.
Keep the macros in msg.h/c where they used to be.  You can still add
your copyright line to those files.
Regarding the macros kept inside msg.c, they are there because we design
by the principle of minimal exposure, even among our module internal files.
Otherwise it is ok.

I don't want to add a new file too, but I found it's hard to define FB_MTU. I
tried to define it in msg.h, and 'crypto.h' should be included, which
'BUF_HEADROOM' is defined in. However, 'msg.h' is already included in
'crypto.h', so it doesn't work.

I tried to define FB_MTU in 'crypto.h', but it feels weird to define
it here. And
FB_MTU is also used in 'bcast.c', so it can't be defined in 'msg.c'.

I will see if there is a better solution.

I think we can leverage the fact that this by definition is a node local
message, and those are never encrypted.
So, if you base FB_MTU on the non-crypto versions of BUF_HEADROOM and
BUF_TAILROOM we should be safe.
That will even give us better utilization of the space available.

Ok, that sounds nice. I'll make a V2 based on that.

Thanks!
Menglong Dong

On Sat, Jun 05, 2021 at 10:25:53AM -0400, Jon Maloy wrote:

On 6/4/21 9:28 PM, Menglong Dong wrote:

quoted

Hello Maloy,

On Sat, Jun 5, 2021 at 3:20 AM Jon Maloy [off-list ref] wrote:

quoted

[...]

quoted

Please don't add any extra file just for this little fix. We have enough
files.
Keep the macros in msg.h/c where they used to be.  You can still add
your copyright line to those files.
Regarding the macros kept inside msg.c, they are there because we design
by the principle of minimal exposure, even among our module internal files.
Otherwise it is ok.

I don't want to add a new file too, but I found it's hard to define FB_MTU. I
tried to define it in msg.h, and 'crypto.h' should be included, which
'BUF_HEADROOM' is defined in. However, 'msg.h' is already included in
'crypto.h', so it doesn't work.

I tried to define FB_MTU in 'crypto.h', but it feels weird to define
it here. And
FB_MTU is also used in 'bcast.c', so it can't be defined in 'msg.c'.

I will see if there is a better solution.

I think we can leverage the fact that this by definition is a node local
message, and those are never encrypted.
So, if you base FB_MTU on the non-crypto versions of BUF_HEADROOM and
BUF_TAILROOM we should be safe.
That will even give us better utilization of the space available.

I think we misunderstanded something. If I base FB_MTU on the non-crypto
version of BUF_TAILROOM, it will eat 2 pages too. Because the
BUF_TAILROOM used in tipc_buf_acquire() is the crypto version, which is
larger than the non-crypto version:

struct sk_buff *tipc_buf_acquire(u32 size, gfp_t gfp)
{
	struct sk_buff *skb;
#ifdef CONFIG_TIPC_CRYPTO
	unsigned int buf_size = (BUF_HEADROOM + size + BUF_TAILROOM + 3) & ~3u;
#else
	unsigned int buf_size = (BUF_HEADROOM + size + 3) & ~3u;
#endif
	[..]
}

So if I use the non-crypto version, the size allocated will be:

  PAGE_SIZE - BUF_HEADROOM_non-crypto - BUF_TAILROOM_non-crypt +
    BUF_HEADROOM_crypto + BUF_TAILROOM_crypto

which is larger than PAGE_SIZE.

So, I think the simple way is to define FB_MTU in 'crypto.h'. Is this
acceptable?

Thanks!
Menglong Dong

On 6/7/21 8:51 AM, Menglong Dong wrote:

On Sat, Jun 05, 2021 at 10:25:53AM -0400, Jon Maloy wrote:

quoted

On 6/4/21 9:28 PM, Menglong Dong wrote:

quoted

Hello Maloy,

On Sat, Jun 5, 2021 at 3:20 AM Jon Maloy [off-list ref] wrote:

quoted

quoted

quoted

[...]

So if I use the non-crypto version, the size allocated will be:

   PAGE_SIZE - BUF_HEADROOM_non-crypto - BUF_TAILROOM_non-crypt +
     BUF_HEADROOM_crypto + BUF_TAILROOM_crypto

which is larger than PAGE_SIZE.

So, I think the simple way is to define FB_MTU in 'crypto.h'. Is this
acceptable?

Thanks!
Menglong Dong

I spent a little more time looking into this. I think the best we can do 
is to keep FB_MTU internal to msg.c, and then add an outline function to 
msg.c that can be used by bcast.c. The way it is used is never time 
critical.

I also see that we could need a little cleanup around this. There is a 
redundant align() function that should be removed and replaced with the 
global ALIGN() macro.
Even tipc_buf_acquire() should use this macro instead of the explicit 
method that is used now.
In general, I stongly dislike conditional code, and it is not necessary 
in this function. If we redefine the non-crypto BUF_TAILROOM to 0 
instead of 16 (it is not used anywhere else) we could get rid of this too.

But I leave that to you. If you only fix the FB_MTU macro I am content.

///jon

On Tue, Jun 08, 2021 at 06:37:38PM -0400, Jon Maloy wrote:

[...]

I spent a little more time looking into this. I think the best we can do is
to keep FB_MTU internal to msg.c, and then add an outline function to msg.c
that can be used by bcast.c. The way it is used is never time critical.

I also see that we could need a little cleanup around this. There is a
redundant align() function that should be removed and replaced with the
global ALIGN() macro.
Even tipc_buf_acquire() should use this macro instead of the explicit method
that is used now.
In general, I stongly dislike conditional code, and it is not necessary in
this function. If we redefine the non-crypto BUF_TAILROOM to 0 instead of 16
(it is not used anywhere else) we could get rid of this too.

But I leave that to you. If you only fix the FB_MTU macro I am content.

Yeah, I think I can handle it, just leave it to me.

(finger heart :/)
Menglong Dong

On 6/8/21 10:54 PM, Menglong Dong wrote:

On Tue, Jun 08, 2021 at 06:37:38PM -0400, Jon Maloy wrote:

quoted

[...]

quoted

I spent a little more time looking into this. I think the best we can do is
to keep FB_MTU internal to msg.c, and then add an outline function to msg.c
that can be used by bcast.c. The way it is used is never time critical.

I also see that we could need a little cleanup around this. There is a
redundant align() function that should be removed and replaced with the
global ALIGN() macro.
Even tipc_buf_acquire() should use this macro instead of the explicit method
that is used now.
In general, I stongly dislike conditional code, and it is not necessary in
this function. If we redefine the non-crypto BUF_TAILROOM to 0 instead of 16
(it is not used anywhere else) we could get rid of this too.

But I leave that to you. If you only fix the FB_MTU macro I am content.

Yeah, I think I can handle it, just leave it to me.

(finger heart :/)
Menglong DongI

It seems like I have been misleading you. It turns out that these 
messages *will* be sent out over the nework in some cases, i.e. at 
multicast/broadcast over an UDP bearer.
So, what we need is two macros, one with the conditional crypto 
head/tailroom defined as you first suggested, and one that only use the 
non-crypto head/tailroom as we have been discussing now.
The first one can be defined inside bcast.c, the latter  inside msg.c.
It might also be a good idea to give the macros more descriptive names, 
such as ONEPAGE_MTU in the broadcast version, and ONEPAGE_SKB in the 
node local
version.

Does that make sense?

///jon

On Wed, Jun 09, 2021 at 03:34:33AM -0400, Jon Maloy wrote:

[...]

It seems like I have been misleading you. It turns out that these messages
*will* be sent out over the nework in some cases, i.e. at
multicast/broadcast over an UDP bearer.
So, what we need is two macros, one with the conditional crypto
head/tailroom defined as you first suggested, and one that only use the
non-crypto head/tailroom as we have been discussing now.
The first one can be defined inside bcast.c, the latter  inside msg.c.
It might also be a good idea to give the macros more descriptive names, such
as ONEPAGE_MTU in the broadcast version, and ONEPAGE_SKB in the node local
version.

Does that make sense?

I think it's another point which can be optimized. However, with
CONFIG_TIPC_CRYPTO=y, the BUF_HEADROOM used in tipc_buf_acquire() is
always the crypto version, so it donen't work to define FB_MTU with
non-crypto BUF_HEADROOM.

So the point is to make a non-crypto version tipc_buf_acquire(), which
is used to alloc data for non-crypto message with non-crypto
BUF_HEADROOM. And that require us to distinguish the message that don't
crypto. Is there simple way to achieve this goal?

(Btw, I resended the patches for the 'two pages' problems.)

Thanks!
Menglong Dong

quoted