| Message ID | 20230525211346.718562-1-Kenny.Ho@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Truncate UTS_RELEASE for rxrpc version | expand |
Kenny Ho <Kenny.Ho@amd.com> wrote: > @@ -30,6 +28,7 @@ static void rxrpc_send_version_request(struct rxrpc_local *local, > struct sockaddr_rxrpc srx; > struct msghdr msg; > struct kvec iov[2]; > + static char rxrpc_version_string[65]; > size_t len; > int ret; > That's not thread-safe. If you have multiple endpoints each one of them could be overwriting the string at the same time. We can't guarantee that one wouldn't corrupt the other. There's also no need to reprint it every time; just once during module init will do. How about the attached patch instead? David --- rxrpc: Truncate UTS_RELEASE for rxrpc version UTS_RELEASE has a maximum length of 64 which can cause rxrpc_version to exceed the 65 byte message limit. Per the rx spec[1]: "If a server receives a packet with a type value of 13, and the client-initiated flag set, it should respond with a 65-byte payload containing a string that identifies the version of AFS software it is running." The current implementation causes a compile error when WERROR is turned on and/or UTS_RELEASE exceeds the length of 49 (making the version string more than 64 characters). Fix this by generating the string during module initialisation and limiting the UTS_RELEASE segment of the string does not exceed 49 chars. We need to make sure that the 64 bytes includes "linux-" at the front and " AF_RXRPC" at the back as this may be used in pattern matching. Fixes: 44ba06987c0b ("RxRPC: Handle VERSION Rx protocol packets") Reported-by: Kenny Ho <Kenny.Ho@amd.com> Link: https://lore.kernel.org/r/20230523223944.691076-1-Kenny.Ho@amd.com/ Signed-off-by: David Howells <dhowells@redhat.com> Link: https://web.mit.edu/kolya/afs/rx/rx-spec [1] cc: Marc Dionne <marc.dionne@auristor.com> cc: Andrew Lunn <andrew@lunn.ch> cc: David Laight <David.Laight@ACULAB.COM> cc: "David S. Miller" <davem@davemloft.net> cc: Eric Dumazet <edumazet@google.com> cc: Jakub Kicinski <kuba@kernel.org> cc: Paolo Abeni <pabeni@redhat.com> cc: linux-afs@lists.infradead.org cc: netdev@vger.kernel.org --- net/rxrpc/af_rxrpc.c | 1 + net/rxrpc/ar-internal.h | 1 + net/rxrpc/local_event.c | 11 ++++++++++- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c index 31f738d65f1c..da0b3b5157d5 100644 --- a/net/rxrpc/af_rxrpc.c +++ b/net/rxrpc/af_rxrpc.c @@ -980,6 +980,7 @@ static int __init af_rxrpc_init(void) BUILD_BUG_ON(sizeof(struct rxrpc_skb_priv) > sizeof_field(struct sk_buff, cb)); ret = -ENOMEM; + rxrpc_gen_version_string(); rxrpc_call_jar = kmem_cache_create( "rxrpc_call_jar", sizeof(struct rxrpc_call), 0, SLAB_HWCACHE_ALIGN, NULL); diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h index 5d44dc08f66d..e8e14c6f904d 100644 --- a/net/rxrpc/ar-internal.h +++ b/net/rxrpc/ar-internal.h @@ -1068,6 +1068,7 @@ int rxrpc_get_server_data_key(struct rxrpc_connection *, const void *, time64_t, /* * local_event.c */ +void rxrpc_gen_version_string(void); void rxrpc_send_version_request(struct rxrpc_local *local, struct rxrpc_host_header *hdr, struct sk_buff *skb); diff --git a/net/rxrpc/local_event.c b/net/rxrpc/local_event.c index 5e69ea6b233d..993c69f97488 100644 --- a/net/rxrpc/local_event.c +++ b/net/rxrpc/local_event.c @@ -16,7 +16,16 @@ #include <generated/utsrelease.h> #include "ar-internal.h" -static const char rxrpc_version_string[65] = "linux-" UTS_RELEASE " AF_RXRPC"; +static char rxrpc_version_string[65]; // "linux-" UTS_RELEASE " AF_RXRPC"; + +/* + * Generate the VERSION packet string. + */ +void rxrpc_gen_version_string(void) +{ + snprintf(rxrpc_version_string, sizeof(rxrpc_version_string), + "linux-%.49s AF_RXRPC", UTS_RELEASE); +} /* * Reply to a version request
On Thu, May 25, 2023 at 6:09 PM David Howells <dhowells@redhat.com> wrote: > There's also no need to reprint it every time; just once during module init > will do. How about the attached patch instead? This makes sense and looks fine to me. I don't know the proper etiquette here, but Acked-by: Kenny Ho <Kenny.Ho@amd.com> Kenny
Kenny Ho <y2kenny@gmail.com> wrote: > This makes sense and looks fine to me. I don't know the proper > etiquette here, but > Acked-by: Kenny Ho <Kenny.Ho@amd.com> If I'm not going to pick the patch up, I tend to use Acked-by when reviewing a patch that touches code I'm a listed maintainer for and Reviewed-by when it's code that I'm not a maintainer for... but the descriptions in: Documentation/process/submitting-patches.rst seem to leave a lot of overlap. David
On Thu, May 25, 2023 at 11:09:14PM +0100, David Howells wrote: > Kenny Ho <Kenny.Ho@amd.com> wrote: > > > @@ -30,6 +28,7 @@ static void rxrpc_send_version_request(struct rxrpc_local *local, > > struct sockaddr_rxrpc srx; > > struct msghdr msg; > > struct kvec iov[2]; > > + static char rxrpc_version_string[65]; > > size_t len; > > int ret; > > > > That's not thread-safe. If you have multiple endpoints each one of them could > be overwriting the string at the same time. We can't guarantee that one > wouldn't corrupt the other. > > There's also no need to reprint it every time; just once during module init > will do. How about the attached patch instead? > > David Thanks David ad Kenny, can we arrange for a formal posting of the patch below? I suspect it will languish otherwise. > --- > rxrpc: Truncate UTS_RELEASE for rxrpc version > > UTS_RELEASE has a maximum length of 64 which can cause rxrpc_version to > exceed the 65 byte message limit. > > Per the rx spec[1]: "If a server receives a packet with a type value of 13, > and the client-initiated flag set, it should respond with a 65-byte payload > containing a string that identifies the version of AFS software it is > running." > > The current implementation causes a compile error when WERROR is turned on > and/or UTS_RELEASE exceeds the length of 49 (making the version string more > than 64 characters). > > Fix this by generating the string during module initialisation and limiting > the UTS_RELEASE segment of the string does not exceed 49 chars. We need to > make sure that the 64 bytes includes "linux-" at the front and " AF_RXRPC" at > the back as this may be used in pattern matching. > > Fixes: 44ba06987c0b ("RxRPC: Handle VERSION Rx protocol packets") > Reported-by: Kenny Ho <Kenny.Ho@amd.com> > Link: https://lore.kernel.org/r/20230523223944.691076-1-Kenny.Ho@amd.com/ > Signed-off-by: David Howells <dhowells@redhat.com> > Link: https://web.mit.edu/kolya/afs/rx/rx-spec [1] > cc: Marc Dionne <marc.dionne@auristor.com> > cc: Andrew Lunn <andrew@lunn.ch> > cc: David Laight <David.Laight@ACULAB.COM> > cc: "David S. Miller" <davem@davemloft.net> > cc: Eric Dumazet <edumazet@google.com> > cc: Jakub Kicinski <kuba@kernel.org> > cc: Paolo Abeni <pabeni@redhat.com> > cc: linux-afs@lists.infradead.org > cc: netdev@vger.kernel.org > --- > net/rxrpc/af_rxrpc.c | 1 + > net/rxrpc/ar-internal.h | 1 + > net/rxrpc/local_event.c | 11 ++++++++++- > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c > index 31f738d65f1c..da0b3b5157d5 100644 > --- a/net/rxrpc/af_rxrpc.c > +++ b/net/rxrpc/af_rxrpc.c > @@ -980,6 +980,7 @@ static int __init af_rxrpc_init(void) > BUILD_BUG_ON(sizeof(struct rxrpc_skb_priv) > sizeof_field(struct sk_buff, cb)); > > ret = -ENOMEM; > + rxrpc_gen_version_string(); > rxrpc_call_jar = kmem_cache_create( > "rxrpc_call_jar", sizeof(struct rxrpc_call), 0, > SLAB_HWCACHE_ALIGN, NULL); > diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h > index 5d44dc08f66d..e8e14c6f904d 100644 > --- a/net/rxrpc/ar-internal.h > +++ b/net/rxrpc/ar-internal.h > @@ -1068,6 +1068,7 @@ int rxrpc_get_server_data_key(struct rxrpc_connection *, const void *, time64_t, > /* > * local_event.c > */ > +void rxrpc_gen_version_string(void); > void rxrpc_send_version_request(struct rxrpc_local *local, > struct rxrpc_host_header *hdr, > struct sk_buff *skb); > diff --git a/net/rxrpc/local_event.c b/net/rxrpc/local_event.c > index 5e69ea6b233d..993c69f97488 100644 > --- a/net/rxrpc/local_event.c > +++ b/net/rxrpc/local_event.c > @@ -16,7 +16,16 @@ > #include <generated/utsrelease.h> > #include "ar-internal.h" > > -static const char rxrpc_version_string[65] = "linux-" UTS_RELEASE " AF_RXRPC"; > +static char rxrpc_version_string[65]; // "linux-" UTS_RELEASE " AF_RXRPC"; > + > +/* > + * Generate the VERSION packet string. > + */ > +void rxrpc_gen_version_string(void) > +{ > + snprintf(rxrpc_version_string, sizeof(rxrpc_version_string), > + "linux-%.49s AF_RXRPC", UTS_RELEASE); > +} > > /* > * Reply to a version request > >
diff --git a/net/rxrpc/local_event.c b/net/rxrpc/local_event.c index 19e929c7c38b..90af6fbb9266 100644 --- a/net/rxrpc/local_event.c +++ b/net/rxrpc/local_event.c @@ -16,8 +16,6 @@ #include <generated/utsrelease.h> #include "ar-internal.h" -static const char rxrpc_version_string[65] = "linux-" UTS_RELEASE " AF_RXRPC"; - /* * Reply to a version request */ @@ -30,6 +28,7 @@ static void rxrpc_send_version_request(struct rxrpc_local *local, struct sockaddr_rxrpc srx; struct msghdr msg; struct kvec iov[2]; + static char rxrpc_version_string[65]; size_t len; int ret; @@ -38,6 +37,12 @@ static void rxrpc_send_version_request(struct rxrpc_local *local, if (rxrpc_extract_addr_from_skb(&srx, skb) < 0) return; + if (!rxrpc_version_string[0]) + snprintf(rxrpc_version_string, + sizeof(rxrpc_version_string), + "linux-%.49s AF_RXRPC", + UTS_RELEASE); + msg.msg_name = &srx.transport; msg.msg_namelen = srx.transport_len; msg.msg_control = NULL;
UTS_RELEASE has maximum length of 64 which can cause rxrpc_version to exceed the 65 byte message limit. Per https://web.mit.edu/kolya/afs/rx/rx-spec "If a server receives a packet with a type value of 13, and the client-initiated flag set, it should respond with a 65-byte payload containing a string that identifies the version of AFS software it is running." Current implementation causes compile error when WERROR is turned on and when UTS_RELEASE exceed the length of 49 (making the version string more than 64 characters.) Signed-off-by: Kenny Ho <Kenny.Ho@amd.com> --- net/rxrpc/local_event.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)