| Message ID | 20240422115541.38548-1-linyunsheng@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] rxrpc: Fix using alignmask being zero for __page_frag_alloc_align() | expand |
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@redhat.com> > CC: Alexander Duyck <alexander.duyck@gmail.com> > Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> > --- > 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); >
Yunsheng Lin <linyunsheng@huawei.com> 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 Ummm... 0 *would be* a valid order-based[*] alignment (pow(2,0) is 1). It might actually make more sense to do that than to pass in the number of bytes, then 0 is the default, but either way works. [*] Other places that take an order-based parameter include things like alloc_pages(). The number of pages being requested is pow(2,order). > + return rxrpc_alloc_data_txbuf(call, min_t(size_t, remain, RXRPC_JUMBO_DATALEN), 1U, gfp); > + return rxrpc_alloc_data_txbuf(call, space, 1U, gfp); The 'U' should be unnecessary. > + data_align = max_t(size_t, data_align, L1_CACHE_BYTES); data_align = umax(data_align, L1_CACHE_BYTES); would be better, I think. Anyway, with the umax change above: Acked-by: David Howells <dhowells@redhat.com>
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);
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. 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@redhat.com> CC: Alexander Duyck <alexander.duyck@gmail.com> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> --- net/rxrpc/insecure.c | 2 +- net/rxrpc/rxkad.c | 2 +- net/rxrpc/txbuf.c | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-)