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

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