| Message ID | 20250205060653.2221165-1-buaajxlj@163.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] af_unix: Refine UNIX domain sockets autobind identifier length | expand |
From: Liang Jie <buaajxlj@163.com> Date: Wed, 5 Feb 2025 14:06:53 +0800 > From: Liang Jie <liangjie@lixiang.com> > > Refines autobind identifier length for UNIX domain sockets, addressing > issues of memory waste and code readability. > > The previous implementation in the unix_autobind function of UNIX domain > sockets used hardcoded values such as 16, 6, and 5 for memory allocation > and setting the length of the autobind identifier, which was not only > inflexible but also led to reduced code clarity. Additionally, allocating > 16 bytes of memory for the autobind path was excessive, given that only 6 > bytes were ultimately used. > > To mitigate these issues, introduces the following changes: > - A new macro AUTOBIND_LEN is defined to clearly represent the total > length of the autobind identifier, which improves code readability and > maintainability. It is set to 6 bytes to accommodate the unique autobind > process identifier. > - Memory allocation for the autobind path is now precisely based on > AUTOBIND_LEN, thereby preventing memory waste. > - The sprintf() function call is updated to dynamically format the > autobind identifier according to the defined length, further enhancing > code consistency and readability. > > The modifications result in a leaner memory footprint and elevated code > quality, ensuring that the functional aspect of autobind behavior in UNIX > domain sockets remains intact. > > Signed-off-by: Liang Jie <liangjie@lixiang.com> > --- > net/unix/af_unix.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index 34945de1fb1f..5dcc55f2e3a1 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -1186,6 +1186,13 @@ static struct sock *unix_find_other(struct net *net, > return sk; > } > > +/* > + * Define the total length of the autobind identifier for UNIX domain sockets. > + * - The first byte distinguishes abstract sockets from filesystem-based sockets. Now it's called pathname socket, but I think we don't need a comment here. We already have enough comment/doc in other places and the man page. $ man 7 unix ... The address consists of a null byte followed by 5 bytes in the character set [0-9a-f]. > + * - The subsequent five bytes store a unique identifier for the autobinding process. > + */ > +#define AUTOBIND_LEN 6 UNIX_AUTOBIND_LEN > + > static int unix_autobind(struct sock *sk) > { > struct unix_sock *u = unix_sk(sk); > @@ -1204,11 +1211,11 @@ static int unix_autobind(struct sock *sk) > > err = -ENOMEM; > addr = kzalloc(sizeof(*addr) + > - offsetof(struct sockaddr_un, sun_path) + 16, GFP_KERNEL); > + offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN, GFP_KERNEL); > if (!addr) > goto out; > > - addr->len = offsetof(struct sockaddr_un, sun_path) + 6; > + addr->len = offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN; > addr->name->sun_family = AF_UNIX; > refcount_set(&addr->refcnt, 1); > > @@ -1217,7 +1224,7 @@ static int unix_autobind(struct sock *sk) > lastnum = ordernum & 0xFFFFF; > retry: > ordernum = (ordernum + 1) & 0xFFFFF; > - sprintf(addr->name->sun_path + 1, "%05x", ordernum); > + sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum); I feel %05 is easier to read. Note that man page mentions 5 bytes. 1 is also hard-coded here, but I don't think we should write sprintf(addr->name->sun_path + UNIX_ABSTRACT_NAME_OFFSET, "%0*x", UNIX_AUTOBIND_LEN - 1, ordernum) > > new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type); > unix_table_double_lock(net, old_hash, new_hash); > -- > 2.25.1
On Wed, 5 Feb 2025 17:28:41 +0900, Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > From: Liang Jie <buaajxlj@163.com> > Date: Wed, 5 Feb 2025 14:06:53 +0800 > > From: Liang Jie <liangjie@lixiang.com> > > > > Refines autobind identifier length for UNIX domain sockets, addressing > > issues of memory waste and code readability. > > > > The previous implementation in the unix_autobind function of UNIX domain > > sockets used hardcoded values such as 16, 6, and 5 for memory allocation > > and setting the length of the autobind identifier, which was not only > > inflexible but also led to reduced code clarity. Additionally, allocating > > 16 bytes of memory for the autobind path was excessive, given that only 6 > > bytes were ultimately used. > > > > To mitigate these issues, introduces the following changes: > > - A new macro AUTOBIND_LEN is defined to clearly represent the total > > length of the autobind identifier, which improves code readability and > > maintainability. It is set to 6 bytes to accommodate the unique autobind > > process identifier. > > - Memory allocation for the autobind path is now precisely based on > > AUTOBIND_LEN, thereby preventing memory waste. > > - The sprintf() function call is updated to dynamically format the > > autobind identifier according to the defined length, further enhancing > > code consistency and readability. > > > > The modifications result in a leaner memory footprint and elevated code > > quality, ensuring that the functional aspect of autobind behavior in UNIX > > domain sockets remains intact. > > > > Signed-off-by: Liang Jie <liangjie@lixiang.com> > > --- > > net/unix/af_unix.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > > index 34945de1fb1f..5dcc55f2e3a1 100644 > > --- a/net/unix/af_unix.c > > +++ b/net/unix/af_unix.c > > @@ -1186,6 +1186,13 @@ static struct sock *unix_find_other(struct net *net, > > return sk; > > } > > > > +/* > > + * Define the total length of the autobind identifier for UNIX domain sockets. > > + * - The first byte distinguishes abstract sockets from filesystem-based sockets. > > Now it's called pathname socket, but I think we don't need a comment here. > We already have enough comment/doc in other places and the man page. > > $ man 7 unix > ... > The address consists of a null byte followed by 5 bytes in the character set [0-9a-f]. > > > > + * - The subsequent five bytes store a unique identifier for the autobinding process. > > + */ > > +#define AUTOBIND_LEN 6 > > UNIX_AUTOBIND_LEN > > > > + > > static int unix_autobind(struct sock *sk) > > { > > struct unix_sock *u = unix_sk(sk); > > @@ -1204,11 +1211,11 @@ static int unix_autobind(struct sock *sk) > > > > err = -ENOMEM; > > addr = kzalloc(sizeof(*addr) + > > - offsetof(struct sockaddr_un, sun_path) + 16, GFP_KERNEL); > > + offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN, GFP_KERNEL); > > if (!addr) > > goto out; > > > > - addr->len = offsetof(struct sockaddr_un, sun_path) + 6; > > + addr->len = offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN; > > addr->name->sun_family = AF_UNIX; > > refcount_set(&addr->refcnt, 1); > > > > @@ -1217,7 +1224,7 @@ static int unix_autobind(struct sock *sk) > > lastnum = ordernum & 0xFFFFF; > > retry: > > ordernum = (ordernum + 1) & 0xFFFFF; > > - sprintf(addr->name->sun_path + 1, "%05x", ordernum); > > + sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum); > > I feel %05 is easier to read. Note that man page mentions 5 bytes. > > 1 is also hard-coded here, but I don't think we should write > > sprintf(addr->name->sun_path + UNIX_ABSTRACT_NAME_OFFSET, > "%0*x", UNIX_AUTOBIND_LEN - 1, ordernum) > Hi Kuniyuki, Thank you very much for your suggestions. I will incorporate them and submit [PATCH v2] accordingly. The logs from 'netdev/build_allmodconfig_warn' indicate that the patch has given rise to the following warning: - ../net/unix/af_unix.c: In function ‘unix_autobind’: - ../net/unix/af_unix.c:1227:48: warning: ‘sprintf’ writing a terminating nul past the end of the destination [-Wformat-overflow=] - 1227 | sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum); - | ^ - ../net/unix/af_unix.c:1227:9: note: ‘sprintf’ output 6 bytes into a destination of size 5 - 1227 | sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum); - | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ It appears that the 'sprintf' call attempts to write a terminating null byte past the end of the 'sun_path' array, potentially causing an overflow. To address this issue, I am considering the following approach: char orderstring[6]; sprintf(orderstring, "%05x", ordernum); memcpy(addr->name->sun_path + 1, orderstring, 5); This would prevent the buffer overflow by using 'memcpy' to safely copy the formatted string into 'sun_path'. Before proceeding with a patch submission, I wanted to consult with you to see if you have any suggestions for a better or more elegant solution to this problem. Thank you for your time and assistance. I look forward to your guidance on this matter. Best regards, Liang Jie > > > > > new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type); > > unix_table_double_lock(net, old_hash, new_hash); > > -- > > 2.25.1
On Wed, Feb 05, 2025 at 02:06:53PM +0800, Liang Jie wrote: > From: Liang Jie <liangjie@lixiang.com> > > Refines autobind identifier length for UNIX domain sockets, addressing > issues of memory waste and code readability. > > The previous implementation in the unix_autobind function of UNIX domain > sockets used hardcoded values such as 16, 6, and 5 for memory allocation > and setting the length of the autobind identifier, which was not only > inflexible but also led to reduced code clarity. Additionally, allocating > 16 bytes of memory for the autobind path was excessive, given that only 6 > bytes were ultimately used. > > To mitigate these issues, introduces the following changes: > - A new macro AUTOBIND_LEN is defined to clearly represent the total > length of the autobind identifier, which improves code readability and > maintainability. It is set to 6 bytes to accommodate the unique autobind > process identifier. > - Memory allocation for the autobind path is now precisely based on > AUTOBIND_LEN, thereby preventing memory waste. > - The sprintf() function call is updated to dynamically format the > autobind identifier according to the defined length, further enhancing > code consistency and readability. > > The modifications result in a leaner memory footprint and elevated code > quality, ensuring that the functional aspect of autobind behavior in UNIX > domain sockets remains intact. > > Signed-off-by: Liang Jie <liangjie@lixiang.com> > --- > net/unix/af_unix.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index 34945de1fb1f..5dcc55f2e3a1 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -1186,6 +1186,13 @@ static struct sock *unix_find_other(struct net *net, > return sk; > } > > +/* > + * Define the total length of the autobind identifier for UNIX domain sockets. > + * - The first byte distinguishes abstract sockets from filesystem-based sockets. > + * - The subsequent five bytes store a unique identifier for the autobinding process. > + */ > +#define AUTOBIND_LEN 6 > + > static int unix_autobind(struct sock *sk) > { > struct unix_sock *u = unix_sk(sk); > @@ -1204,11 +1211,11 @@ static int unix_autobind(struct sock *sk) > > err = -ENOMEM; > addr = kzalloc(sizeof(*addr) + > - offsetof(struct sockaddr_un, sun_path) + 16, GFP_KERNEL); > + offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN, GFP_KERNEL); Hi Liang Jie, 1. While we are here, can we try to move this code to respect the preference for lines 80 columns wide or less in Networking code? e.g. addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN, GFP_KERNEL); 2. More importantly, this allocates AUTOBIND_LEN bytes for sun_path. However, because the sprintf() will append a trailing '\0' it will write up to AUTOBIND_LEN (that is, AUTOBIND_LEN - 1 + 1) bytes at an offset of 1 to sun_path. IOW, the write may be one byte larger than the buffer with the '\0' overflowing. Flagged by W=1 build with gcc-14 > if (!addr) > goto out; > > - addr->len = offsetof(struct sockaddr_un, sun_path) + 6; > + addr->len = offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN; > addr->name->sun_family = AF_UNIX; > refcount_set(&addr->refcnt, 1); > > @@ -1217,7 +1224,7 @@ static int unix_autobind(struct sock *sk) > lastnum = ordernum & 0xFFFFF; > retry: > ordernum = (ordernum + 1) & 0xFFFFF; > - sprintf(addr->name->sun_path + 1, "%05x", ordernum); > + sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum); > > new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type); > unix_table_double_lock(net, old_hash, new_hash);
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 34945de1fb1f..5dcc55f2e3a1 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1186,6 +1186,13 @@ static struct sock *unix_find_other(struct net *net, return sk; } +/* + * Define the total length of the autobind identifier for UNIX domain sockets. + * - The first byte distinguishes abstract sockets from filesystem-based sockets. + * - The subsequent five bytes store a unique identifier for the autobinding process. + */ +#define AUTOBIND_LEN 6 + static int unix_autobind(struct sock *sk) { struct unix_sock *u = unix_sk(sk); @@ -1204,11 +1211,11 @@ static int unix_autobind(struct sock *sk) err = -ENOMEM; addr = kzalloc(sizeof(*addr) + - offsetof(struct sockaddr_un, sun_path) + 16, GFP_KERNEL); + offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN, GFP_KERNEL); if (!addr) goto out; - addr->len = offsetof(struct sockaddr_un, sun_path) + 6; + addr->len = offsetof(struct sockaddr_un, sun_path) + AUTOBIND_LEN; addr->name->sun_family = AF_UNIX; refcount_set(&addr->refcnt, 1); @@ -1217,7 +1224,7 @@ static int unix_autobind(struct sock *sk) lastnum = ordernum & 0xFFFFF; retry: ordernum = (ordernum + 1) & 0xFFFFF; - sprintf(addr->name->sun_path + 1, "%05x", ordernum); + sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum); new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type); unix_table_double_lock(net, old_hash, new_hash);