Date:   Fri, 4 Jun 2021 15:20:16 -0400
From:   Jon Maloy <jmaloy@...hat.com>
To:     menglong8.dong@...il.com
Cc:     ying.xue@...driver.com, davem@...emloft.net, kuba@...nel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        tipc-discussion@...ts.sourceforge.net,
        Menglong Dong <dong.menglong@....com.cn>,
        Zeal Robot <zealci@....com.cn>
Subject: Re: [PATCH net-next] net: tipc: fix FB_MTU eat two pages



On 6/4/21 3:44 AM, menglong8.dong@...il.com wrote:
> From: Menglong Dong <dong.menglong@....com.cn>
>
> 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 <zealci@....com.cn>
> Signed-off-by: Menglong Dong <dong.menglong@....com.cn>
> ---
>   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
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

> @@ -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

Powered by blists - more mailing lists