Re: [PATCH net] rxrpc: Fix using alignmask being zero for __page_frag_alloc_align()

[Yunsheng Lin]

On 2024/4/22 19:55, Yunsheng Lin wrote:
> rxrpc_alloc_data_txbuf() may be called with data_align being
> zero in none_alloc_txbuf() and rxkad_alloc_txbuf(), data_align
> is supposed to be an order-based alignment value, but zero is
> not a valid order-based alignment value, and '~(data_align - 1)'
> doesn't result in a valid mask-based alignment value for
> __page_frag_alloc_align().
> 
> Fix it by passing a valid order-based alignment value in
> none_alloc_txbuf() and rxkad_alloc_txbuf().
> 
> Also use page_frag_alloc_align() expecting an order-based
> alignment value in rxrpc_alloc_data_txbuf() to avoid doing the
> alignment converting operation and to catch possible invalid
> alignment value in the future. Remove the 'if (data_align)'
> checking too, as it is always true for a valid order-based
> alignment value.
> 

Hi, All

Looking at the entry in MAINTAINERS, it seems this patch is
supposed to go through David Howells's tree if this patch is
ok to go as the state of this patch in netdevbpf' patchwork
is changed to 'Not Applicable'?

@David, please take a look if this patch is ok?

> Fixes: 6b2536462fd4 ("rxrpc: Fix use of changed alignment param to page_frag_alloc_align()")
> Fixes: 49489bb03a50 ("rxrpc: Do zerocopy using MSG_SPLICE_PAGES and page frags")
> CC: David Howells <dhowells@xxxxxxxxxx>
> CC: Alexander Duyck <alexander.duyck@xxxxxxxxx>
> Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
> ---
>  net/rxrpc/insecure.c |  2 +-
>  net/rxrpc/rxkad.c    |  2 +-
>  net/rxrpc/txbuf.c    | 10 +++++-----
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/net/rxrpc/insecure.c b/net/rxrpc/insecure.c
> index f2701068ed9e..b52b75a0fdac 100644
> --- a/net/rxrpc/insecure.c
> +++ b/net/rxrpc/insecure.c
> @@ -19,7 +19,7 @@ static int none_init_connection_security(struct rxrpc_connection *conn,
>   */
>  static struct rxrpc_txbuf *none_alloc_txbuf(struct rxrpc_call *call, size_t remain, gfp_t gfp)
>  {
> -	return rxrpc_alloc_data_txbuf(call, min_t(size_t, remain, RXRPC_JUMBO_DATALEN), 0, gfp);
> +	return rxrpc_alloc_data_txbuf(call, min_t(size_t, remain, RXRPC_JUMBO_DATALEN), 1U, gfp);
>  }
>  
>  static int none_secure_packet(struct rxrpc_call *call, struct rxrpc_txbuf *txb)
> diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
> index f1a68270862d..c48e93a26b2a 100644
> --- a/net/rxrpc/rxkad.c
> +++ b/net/rxrpc/rxkad.c
> @@ -155,7 +155,7 @@ static struct rxrpc_txbuf *rxkad_alloc_txbuf(struct rxrpc_call *call, size_t rem
>  	switch (call->conn->security_level) {
>  	default:
>  		space = min_t(size_t, remain, RXRPC_JUMBO_DATALEN);
> -		return rxrpc_alloc_data_txbuf(call, space, 0, gfp);
> +		return rxrpc_alloc_data_txbuf(call, space, 1U, gfp);
>  	case RXRPC_SECURITY_AUTH:
>  		shdr = sizeof(struct rxkad_level1_hdr);
>  		break;
> diff --git a/net/rxrpc/txbuf.c b/net/rxrpc/txbuf.c
> index e0679658d9de..994d6582d5e2 100644
> --- a/net/rxrpc/txbuf.c
> +++ b/net/rxrpc/txbuf.c
> @@ -21,20 +21,20 @@ struct rxrpc_txbuf *rxrpc_alloc_data_txbuf(struct rxrpc_call *call, size_t data_
>  {
>  	struct rxrpc_wire_header *whdr;
>  	struct rxrpc_txbuf *txb;
> -	size_t total, hoff = 0;
> +	size_t total, hoff;
>  	void *buf;
>  
>  	txb = kmalloc(sizeof(*txb), gfp);
>  	if (!txb)
>  		return NULL;
>  
> -	if (data_align)
> -		hoff = round_up(sizeof(*whdr), data_align) - sizeof(*whdr);
> +	hoff = round_up(sizeof(*whdr), data_align) - sizeof(*whdr);
>  	total = hoff + sizeof(*whdr) + data_size;
>  
> +	data_align = max_t(size_t, data_align, L1_CACHE_BYTES);
>  	mutex_lock(&call->conn->tx_data_alloc_lock);
> -	buf = __page_frag_alloc_align(&call->conn->tx_data_alloc, total, gfp,
> -				      ~(data_align - 1) & ~(L1_CACHE_BYTES - 1));
> +	buf = page_frag_alloc_align(&call->conn->tx_data_alloc, total, gfp,
> +				    data_align);
>  	mutex_unlock(&call->conn->tx_data_alloc_lock);
>  	if (!buf) {
>  		kfree(txb);
>