af_unix: Refine UNIX pathname sockets autobind identifier length

[PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

Posted by Liang Jie 10 months, 1 week ago

From: Liang Jie <liangjie@lixiang.com>

Refines autobind identifier length for UNIX pathname sockets, addressing
issues of memory waste and code readability.

The previous implementation in the unix_autobind function of UNIX pathname
sockets used hardcoded values such as 16 and 6 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 UNIX_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
   UNIX_AUTOBIND_LEN, thereby preventing memory waste.
 - To avoid buffer overflow and ensure that only the intended number of
   bytes are written, sprintf is replaced by snprintf with the proper
   buffer size set explicitly.

The modifications result in a leaner memory footprint and elevated code
quality, ensuring that the functional aspect of autobind behavior in UNIX
pathname sockets remains intact.

Signed-off-by: Liang Jie <liangjie@lixiang.com>
Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---

Changes in v2:
 - Removed the comments describing AUTOBIND_LEN.
 - Renamed the macro AUTOBIND_LEN to UNIX_AUTOBIND_LEN for clarity and
   specificity.
 - Corrected the buffer length in snprintf to prevent potential buffer
   overflow issues.
 - Addressed warning from checkpatch.
 - Link to v1: https://lore.kernel.org/all/20250205060653.2221165-1-buaajxlj@163.com/

 net/unix/af_unix.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 34945de1fb1f..6c449f78f0a6 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1186,6 +1186,8 @@ static struct sock *unix_find_other(struct net *net,
 	return sk;
 }
 
+#define UNIX_AUTOBIND_LEN 6
+
 static int unix_autobind(struct sock *sk)
 {
 	struct unix_sock *u = unix_sk(sk);
@@ -1203,12 +1205,12 @@ static int unix_autobind(struct sock *sk)
 		goto out;
 
 	err = -ENOMEM;
-	addr = kzalloc(sizeof(*addr) +
-		       offsetof(struct sockaddr_un, sun_path) + 16, GFP_KERNEL);
+	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
+			UNIX_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) + UNIX_AUTOBIND_LEN;
 	addr->name->sun_family = AF_UNIX;
 	refcount_set(&addr->refcnt, 1);
 
@@ -1217,7 +1219,7 @@ static int unix_autobind(struct sock *sk)
 	lastnum = ordernum & 0xFFFFF;
 retry:
 	ordernum = (ordernum + 1) & 0xFFFFF;
-	sprintf(addr->name->sun_path + 1, "%05x", ordernum);
+	snprintf(addr->name->sun_path + 1, 5, "%05x", 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

Re: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

Posted by kernel test robot 10 months, 1 week ago

Hi Liang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Liang-Jie/af_unix-Refine-UNIX-pathname-sockets-autobind-identifier-length/20250206-134846
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250206054451.4070941-1-buaajxlj%40163.com
patch subject: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length
config: hexagon-randconfig-001-20250207 (https://download.01.org/0day-ci/archive/20250209/202502090056.Rl1rtpr5-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 6807164500e9920638e2ab0cdb4bf8321d24f8eb)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250209/202502090056.Rl1rtpr5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502090056.Rl1rtpr5-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/unix/af_unix.c:1222:2: warning: 'snprintf' will always be truncated; specified size is 5, but format string expands to at least 6 [-Wformat-truncation]
    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
         |         ^
   1 warning generated.


vim +/snprintf +1222 net/unix/af_unix.c

  1190	
  1191	static int unix_autobind(struct sock *sk)
  1192	{
  1193		struct unix_sock *u = unix_sk(sk);
  1194		unsigned int new_hash, old_hash;
  1195		struct net *net = sock_net(sk);
  1196		struct unix_address *addr;
  1197		u32 lastnum, ordernum;
  1198		int err;
  1199	
  1200		err = mutex_lock_interruptible(&u->bindlock);
  1201		if (err)
  1202			return err;
  1203	
  1204		if (u->addr)
  1205			goto out;
  1206	
  1207		err = -ENOMEM;
  1208		addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
  1209				UNIX_AUTOBIND_LEN, GFP_KERNEL);
  1210		if (!addr)
  1211			goto out;
  1212	
  1213		addr->len = offsetof(struct sockaddr_un, sun_path) + UNIX_AUTOBIND_LEN;
  1214		addr->name->sun_family = AF_UNIX;
  1215		refcount_set(&addr->refcnt, 1);
  1216	
  1217		old_hash = sk->sk_hash;
  1218		ordernum = get_random_u32();
  1219		lastnum = ordernum & 0xFFFFF;
  1220	retry:
  1221		ordernum = (ordernum + 1) & 0xFFFFF;
> 1222		snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
  1223	
  1224		new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type);
  1225		unix_table_double_lock(net, old_hash, new_hash);
  1226	
  1227		if (__unix_find_socket_byname(net, addr->name, addr->len, new_hash)) {
  1228			unix_table_double_unlock(net, old_hash, new_hash);
  1229	
  1230			/* __unix_find_socket_byname() may take long time if many names
  1231			 * are already in use.
  1232			 */
  1233			cond_resched();
  1234	
  1235			if (ordernum == lastnum) {
  1236				/* Give up if all names seems to be in use. */
  1237				err = -ENOSPC;
  1238				unix_release_addr(addr);
  1239				goto out;
  1240			}
  1241	
  1242			goto retry;
  1243		}
  1244	
  1245		__unix_set_addr_hash(net, sk, addr, new_hash);
  1246		unix_table_double_unlock(net, old_hash, new_hash);
  1247		err = 0;
  1248	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

Posted by kernel test robot 10 months, 1 week ago

Hi Liang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Liang-Jie/af_unix-Refine-UNIX-pathname-sockets-autobind-identifier-length/20250206-134846
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250206054451.4070941-1-buaajxlj%40163.com
patch subject: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length
config: csky-randconfig-001-20250207 (https://download.01.org/0day-ci/archive/20250209/202502090018.NcW3Qcd3-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250209/202502090018.NcW3Qcd3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502090018.NcW3Qcd3-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/unix/af_unix.c: In function 'unix_autobind':
>> net/unix/af_unix.c:1222:52: warning: 'snprintf' output truncated before the last format character [-Wformat-truncation=]
    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
         |                                                    ^
   net/unix/af_unix.c:1222:9: note: 'snprintf' output 6 bytes into a destination of size 5
    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/snprintf +1222 net/unix/af_unix.c

  1190	
  1191	static int unix_autobind(struct sock *sk)
  1192	{
  1193		struct unix_sock *u = unix_sk(sk);
  1194		unsigned int new_hash, old_hash;
  1195		struct net *net = sock_net(sk);
  1196		struct unix_address *addr;
  1197		u32 lastnum, ordernum;
  1198		int err;
  1199	
  1200		err = mutex_lock_interruptible(&u->bindlock);
  1201		if (err)
  1202			return err;
  1203	
  1204		if (u->addr)
  1205			goto out;
  1206	
  1207		err = -ENOMEM;
  1208		addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
  1209				UNIX_AUTOBIND_LEN, GFP_KERNEL);
  1210		if (!addr)
  1211			goto out;
  1212	
  1213		addr->len = offsetof(struct sockaddr_un, sun_path) + UNIX_AUTOBIND_LEN;
  1214		addr->name->sun_family = AF_UNIX;
  1215		refcount_set(&addr->refcnt, 1);
  1216	
  1217		old_hash = sk->sk_hash;
  1218		ordernum = get_random_u32();
  1219		lastnum = ordernum & 0xFFFFF;
  1220	retry:
  1221		ordernum = (ordernum + 1) & 0xFFFFF;
> 1222		snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
  1223	
  1224		new_hash = unix_abstract_hash(addr->name, addr->len, sk->sk_type);
  1225		unix_table_double_lock(net, old_hash, new_hash);
  1226	
  1227		if (__unix_find_socket_byname(net, addr->name, addr->len, new_hash)) {
  1228			unix_table_double_unlock(net, old_hash, new_hash);
  1229	
  1230			/* __unix_find_socket_byname() may take long time if many names
  1231			 * are already in use.
  1232			 */
  1233			cond_resched();
  1234	
  1235			if (ordernum == lastnum) {
  1236				/* Give up if all names seems to be in use. */
  1237				err = -ENOSPC;
  1238				unix_release_addr(addr);
  1239				goto out;
  1240			}
  1241	
  1242			goto retry;
  1243		}
  1244	
  1245		__unix_set_addr_hash(net, sk, addr, new_hash);
  1246		unix_table_double_unlock(net, old_hash, new_hash);
  1247		err = 0;
  1248	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

Posted by Liang Jie 10 months, 1 week ago

Hi Kuniyuki,

The logs from 'netdev/build_allmodconfig_warn' is as follows:
  ../net/unix/af_unix.c: In function ‘unix_autobind’:
  ../net/unix/af_unix.c:1222:52: warning: ‘snprintf’ output truncated before the last format character [-Wformat-truncation=]
   1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
        |                                                    ^
  ../net/unix/af_unix.c:1222:9: note: ‘snprintf’ output 6 bytes into a destination of size 5
   1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

snprintf() also append a trailing '\0' at the end of the sun_path.

Now, I think of three options. Which one do you think we should choose?

1. Allocate an additional byte during the kzalloc phase.
	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
		       UNIX_AUTOBIND_LEN + 1, GFP_KERNEL);

2. Use temp buffer and memcpy() for handling.

3. Keep the current code as it is.

Do you have any other suggestions?

Best regards,
Liang

> From: Liang Jie <liangjie@lixiang.com>
> 
> Refines autobind identifier length for UNIX pathname sockets, addressing
> issues of memory waste and code readability.
> 
> The previous implementation in the unix_autobind function of UNIX pathname
> sockets used hardcoded values such as 16 and 6 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 UNIX_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
>    UNIX_AUTOBIND_LEN, thereby preventing memory waste.
>  - To avoid buffer overflow and ensure that only the intended number of
>    bytes are written, sprintf is replaced by snprintf with the proper
>    buffer size set explicitly.
> 
> The modifications result in a leaner memory footprint and elevated code
> quality, ensuring that the functional aspect of autobind behavior in UNIX
> pathname sockets remains intact.
> 
> Signed-off-by: Liang Jie <liangjie@lixiang.com>
> Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
> 
> Changes in v2:
>  - Removed the comments describing AUTOBIND_LEN.
>  - Renamed the macro AUTOBIND_LEN to UNIX_AUTOBIND_LEN for clarity and
>    specificity.
>  - Corrected the buffer length in snprintf to prevent potential buffer
>    overflow issues.
>  - Addressed warning from checkpatch.
>  - Link to v1: https://lore.kernel.org/all/20250205060653.2221165-1-buaajxlj@163.com/
> 
>  net/unix/af_unix.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 34945de1fb1f..6c449f78f0a6 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1186,6 +1186,8 @@ static struct sock *unix_find_other(struct net *net,
>  	return sk;
>  }
>  
> +#define UNIX_AUTOBIND_LEN 6
> +
>  static int unix_autobind(struct sock *sk)
>  {
>  	struct unix_sock *u = unix_sk(sk);
> @@ -1203,12 +1205,12 @@ static int unix_autobind(struct sock *sk)
>  		goto out;
>  
>  	err = -ENOMEM;
> -	addr = kzalloc(sizeof(*addr) +
> -		       offsetof(struct sockaddr_un, sun_path) + 16, GFP_KERNEL);
> +	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
> +			UNIX_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) + UNIX_AUTOBIND_LEN;
>  	addr->name->sun_family = AF_UNIX;
>  	refcount_set(&addr->refcnt, 1);
>  
> @@ -1217,7 +1219,7 @@ static int unix_autobind(struct sock *sk)
>  	lastnum = ordernum & 0xFFFFF;
>  retry:
>  	ordernum = (ordernum + 1) & 0xFFFFF;
> -	sprintf(addr->name->sun_path + 1, "%05x", ordernum);
> +	snprintf(addr->name->sun_path + 1, 5, "%05x", 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

Re: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

Posted by Kuniyuki Iwashima 10 months, 1 week ago

From: Liang Jie <buaajxlj@163.com>
Date: Thu,  6 Feb 2025 16:19:05 +0800
> Hi Kuniyuki,
> 
> The logs from 'netdev/build_allmodconfig_warn' is as follows:
>   ../net/unix/af_unix.c: In function ‘unix_autobind’:
>   ../net/unix/af_unix.c:1222:52: warning: ‘snprintf’ output truncated before the last format character [-Wformat-truncation=]
>    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
>         |                                                    ^
>   ../net/unix/af_unix.c:1222:9: note: ‘snprintf’ output 6 bytes into a destination of size 5
>    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> snprintf() also append a trailing '\0' at the end of the sun_path.

I didn't say snprintf() would work rather we need a variant of it that
does not terminate string with \0.


> 
> Now, I think of three options. Which one do you think we should choose?
> 
> 1. Allocate an additional byte during the kzalloc phase.
> 	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
> 		       UNIX_AUTOBIND_LEN + 1, GFP_KERNEL);
> 
> 2. Use temp buffer and memcpy() for handling.
> 
> 3. Keep the current code as it is.
> 
> Do you have any other suggestions?

I'd choose 3. as said in v1 thread.  We can't avoid hard-coding and
adjustment like +1 and -1 here.

Re: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

Posted by Liang Jie 10 months, 1 week ago

From: Kuniyuki Iwashima <kuniyu@amazon.com>
Date: Thu, 6 Feb 2025 17:58:34 +0900
> From: Liang Jie <buaajxlj@163.com>
> Date: Thu,  6 Feb 2025 16:19:05 +0800
> > Hi Kuniyuki,
> > 
> > The logs from 'netdev/build_allmodconfig_warn' is as follows:
> >   ../net/unix/af_unix.c: In function ‘unix_autobind’:
> >   ../net/unix/af_unix.c:1222:52: warning: ‘snprintf’ output truncated before the last format character [-Wformat-truncation=]
> >    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
> >         |                                                    ^
> >   ../net/unix/af_unix.c:1222:9: note: ‘snprintf’ output 6 bytes into a destination of size 5
> >    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
> >         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > snprintf() also append a trailing '\0' at the end of the sun_path.
> 
> I didn't say snprintf() would work rather we need a variant of it that
> does not terminate string with \0.
> 
> 
> > 
> > Now, I think of three options. Which one do you think we should choose?
> > 
> > 1. Allocate an additional byte during the kzalloc phase.
> > 	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
> > 		       UNIX_AUTOBIND_LEN + 1, GFP_KERNEL);
> > 
> > 2. Use temp buffer and memcpy() for handling.
> > 
> > 3. Keep the current code as it is.
> > 
> > Do you have any other suggestions?
> 
> I'd choose 3. as said in v1 thread.  We can't avoid hard-coding and
> adjustment like +1 and -1 here.

The option 3 would result in a waste of ten bytes. Why not choose option 1.

I have a question about the initial use of the hardcoded value 16.
Why was this value chosen and not another?  sizeof(struct sockaddr)?

Its introduction felt abrupt and made understanding the code challenging for me,
which is why I submitted a patch to address this.

Re: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

Posted by Eric Dumazet 10 months, 1 week ago

On Thu, Feb 6, 2025 at 10:45 AM Liang Jie <buaajxlj@163.com> wrote:
>
> From: Kuniyuki Iwashima <kuniyu@amazon.com>
> Date: Thu, 6 Feb 2025 17:58:34 +0900
> > From: Liang Jie <buaajxlj@163.com>
> > Date: Thu,  6 Feb 2025 16:19:05 +0800
> > > Hi Kuniyuki,
> > >
> > > The logs from 'netdev/build_allmodconfig_warn' is as follows:
> > >   ../net/unix/af_unix.c: In function ‘unix_autobind’:
> > >   ../net/unix/af_unix.c:1222:52: warning: ‘snprintf’ output truncated before the last format character [-Wformat-truncation=]
> > >    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
> > >         |                                                    ^
> > >   ../net/unix/af_unix.c:1222:9: note: ‘snprintf’ output 6 bytes into a destination of size 5
> > >    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
> > >         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > snprintf() also append a trailing '\0' at the end of the sun_path.
> >
> > I didn't say snprintf() would work rather we need a variant of it that
> > does not terminate string with \0.
> >
> >
> > >
> > > Now, I think of three options. Which one do you think we should choose?
> > >
> > > 1. Allocate an additional byte during the kzalloc phase.
> > >     addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
> > >                    UNIX_AUTOBIND_LEN + 1, GFP_KERNEL);
> > >
> > > 2. Use temp buffer and memcpy() for handling.
> > >
> > > 3. Keep the current code as it is.
> > >
> > > Do you have any other suggestions?
> >
> > I'd choose 3. as said in v1 thread.  We can't avoid hard-coding and
> > adjustment like +1 and -1 here.
>
> The option 3 would result in a waste of ten bytes. Why not choose option 1.
>
> I have a question about the initial use of the hardcoded value 16.
> Why was this value chosen and not another?  sizeof(struct sockaddr)?
>
> Its introduction felt abrupt and made understanding the code challenging for me,
> which is why I submitted a patch to address this.

To be clear, we are discussing here about using kmalloc-16 slab
instead of kmalloc-32

I find this a bit distracting to be honest, given the cost of a file +
inode + af_unix socket.

IMO it might be more interesting to document why abstract sockets
names are limited to 5 hex digits...

Re: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

Posted by Kuniyuki Iwashima 10 months, 1 week ago

> Subject: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

s/pathname/abstract/

In the v1 thread, I meant "filesystem-based sockets" is now called
pathname sockets.  sockets whose name starts with \0 are abstract
sockets.


From: Liang Jie <buaajxlj@163.com>
Date: Thu,  6 Feb 2025 13:44:51 +0800
> Refines autobind identifier length for UNIX pathname sockets, addressing

same here, abstract sockets.


> issues of memory waste and code readability.
> 
> The previous implementation in the unix_autobind function of UNIX pathname
> sockets used hardcoded values such as 16 and 6 for memory allocation and

nit: 6 isn't used for mem alloc.


> setting the length of the autobind identifier, which was not only
> inflexible but also led to reduced code clarity. Additionally, allocating

you need not mention inflexibility as the length are fixed and won't be
changed (it was changed once though)


> 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 UNIX_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
>    UNIX_AUTOBIND_LEN, thereby preventing memory waste.
>  - To avoid buffer overflow and ensure that only the intended number of
>    bytes are written, sprintf is replaced by snprintf with the proper
>    buffer size set explicitly.
> 
> The modifications result in a leaner memory footprint and elevated code
> quality, ensuring that the functional aspect of autobind behavior in UNIX
> pathname sockets remains intact.

s/pathname/abstract/

Overall, the commit message is a bit wordy.  It can be simplified just like

  unix_autobind() allocates 16 bytes but uses 6 bytes only.

  Let's allocate 6 bytes only and use snprintf() to avoid
  unwanted null-termination.


> @@ -1203,12 +1205,12 @@ static int unix_autobind(struct sock *sk)
>  		goto out;
>  
>  	err = -ENOMEM;
> -	addr = kzalloc(sizeof(*addr) +
> -		       offsetof(struct sockaddr_un, sun_path) + 16, GFP_KERNEL);
> +	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
> +			UNIX_AUTOBIND_LEN, GFP_KERNEL);

nit: indent is wrong here.

If you are using emacs, add the following to your config:

(setq-default c-default-style "linux")