Commit 9bf4e919ccad worked around an issue introduced after an innocuous optimisation change in LLVM main:
len is defined as an 'int' because it is assigned from '__user int *optlen'. However, it is clamped against the result of sizeof(), which has a type of 'size_t' ('unsigned long' for 64-bit platforms). This is done with min_t() because min() requires compatible types, which results in both len and the result of sizeof() being casted to 'unsigned int', meaning len changes signs and the result of sizeof() is truncated. From there, len is passed to copy_to_user(), which has a third parameter type of 'unsigned long', so it is widened and changes signs again. This excessive casting in combination with the KCSAN instrumentation causes LLVM to fail to eliminate the __bad_copy_from() call, failing the build.
The same issue occurs in rfcomm in functions rfcomm_sock_bind and rfcomm_sock_getsockopt_old.
Change the type of len to size_t in both rfcomm_sock_bind and rfcomm_sock_getsockopt_old and replace min_t() with min().
Cc: stable@vger.kernel.org Fixes: 9bf4e919ccad ("Bluetooth: Fix type of len in {l2cap,sco}_sock_getsockopt_old()") Link: https://github.com/ClangBuiltLinux/linux/issues/2007 Link: https://github.com/llvm/llvm-project/issues/85647 Signed-off-by: Andrej Shadura andrew.shadura@collabora.co.uk --- net/bluetooth/rfcomm/sock.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c index 37d63d768afb..c0fe96673b3c 100644 --- a/net/bluetooth/rfcomm/sock.c +++ b/net/bluetooth/rfcomm/sock.c @@ -328,14 +328,15 @@ static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr { struct sockaddr_rc sa; struct sock *sk = sock->sk; - int len, err = 0; + int err = 0; + size_t len;
if (!addr || addr_len < offsetofend(struct sockaddr, sa_family) || addr->sa_family != AF_BLUETOOTH) return -EINVAL;
memset(&sa, 0, sizeof(sa)); - len = min_t(unsigned int, sizeof(sa), addr_len); + len = min(sizeof(sa), addr_len); memcpy(&sa, addr, len);
BT_DBG("sk %p %pMR", sk, &sa.rc_bdaddr); @@ -729,7 +730,8 @@ static int rfcomm_sock_getsockopt_old(struct socket *sock, int optname, char __u struct sock *l2cap_sk; struct l2cap_conn *conn; struct rfcomm_conninfo cinfo; - int len, err = 0; + int err = 0; + size_t len; u32 opt;
BT_DBG("sk %p", sk); @@ -783,7 +785,7 @@ static int rfcomm_sock_getsockopt_old(struct socket *sock, int optname, char __u cinfo.hci_handle = conn->hcon->handle; memcpy(cinfo.dev_class, conn->hcon->dev_class, 3);
- len = min_t(unsigned int, len, sizeof(cinfo)); + len = min(len, sizeof(cinfo)); if (copy_to_user(optval, (char *) &cinfo, len)) err = -EFAULT;
Hi Andrej,
On Wed, Oct 02, 2024 at 04:12:17PM +0200, Andrej Shadura wrote:
Commit 9bf4e919ccad worked around an issue introduced after an innocuous optimisation change in LLVM main:
len is defined as an 'int' because it is assigned from '__user int *optlen'. However, it is clamped against the result of sizeof(), which has a type of 'size_t' ('unsigned long' for 64-bit platforms). This is done with min_t() because min() requires compatible types, which results in both len and the result of sizeof() being casted to 'unsigned int', meaning len changes signs and the result of sizeof() is truncated. From there, len is passed to copy_to_user(), which has a third parameter type of 'unsigned long', so it is widened and changes signs again. This excessive casting in combination with the KCSAN instrumentation causes LLVM to fail to eliminate the __bad_copy_from() call, failing the build.
The same issue occurs in rfcomm in functions rfcomm_sock_bind and rfcomm_sock_getsockopt_old.
Was this found by inspection or is there an actual instance of the same warning? For what it's worth, I haven't seen a warning from this file in any of my build tests.
Change the type of len to size_t in both rfcomm_sock_bind and rfcomm_sock_getsockopt_old and replace min_t() with min().
Cc: stable@vger.kernel.org Fixes: 9bf4e919ccad ("Bluetooth: Fix type of len in {l2cap,sco}_sock_getsockopt_old()")
I am not sure that I totally agree with this Fixes tag since I did not have a warning to fix but I guess it makes sense to help with backporting.
Link: https://github.com/ClangBuiltLinux/linux/issues/2007 Link: https://github.com/llvm/llvm-project/issues/85647 Signed-off-by: Andrej Shadura andrew.shadura@collabora.co.uk
Reviewed-by: Nathan Chancellor nathan@kernel.org
net/bluetooth/rfcomm/sock.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c index 37d63d768afb..c0fe96673b3c 100644 --- a/net/bluetooth/rfcomm/sock.c +++ b/net/bluetooth/rfcomm/sock.c @@ -328,14 +328,15 @@ static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr { struct sockaddr_rc sa; struct sock *sk = sock->sk;
- int len, err = 0;
- int err = 0;
- size_t len;
if (!addr || addr_len < offsetofend(struct sockaddr, sa_family) || addr->sa_family != AF_BLUETOOTH) return -EINVAL; memset(&sa, 0, sizeof(sa));
- len = min_t(unsigned int, sizeof(sa), addr_len);
- len = min(sizeof(sa), addr_len); memcpy(&sa, addr, len);
BT_DBG("sk %p %pMR", sk, &sa.rc_bdaddr); @@ -729,7 +730,8 @@ static int rfcomm_sock_getsockopt_old(struct socket *sock, int optname, char __u struct sock *l2cap_sk; struct l2cap_conn *conn; struct rfcomm_conninfo cinfo;
- int len, err = 0;
- int err = 0;
- size_t len; u32 opt;
BT_DBG("sk %p", sk); @@ -783,7 +785,7 @@ static int rfcomm_sock_getsockopt_old(struct socket *sock, int optname, char __u cinfo.hci_handle = conn->hcon->handle; memcpy(cinfo.dev_class, conn->hcon->dev_class, 3);
len = min_t(unsigned int, len, sizeof(cinfo));
len = min(len, sizeof(cinfo));2.43.0
Hi Nathan,
On 02/10/2024 17:22, Nathan Chancellor wrote:
Hi Andrej,
On Wed, Oct 02, 2024 at 04:12:17PM +0200, Andrej Shadura wrote:
Commit 9bf4e919ccad worked around an issue introduced after an innocuous optimisation change in LLVM main:
len is defined as an 'int' because it is assigned from '__user int *optlen'. However, it is clamped against the result of sizeof(), which has a type of 'size_t' ('unsigned long' for 64-bit platforms). This is done with min_t() because min() requires compatible types, which results in both len and the result of sizeof() being casted to 'unsigned int', meaning len changes signs and the result of sizeof() is truncated. From there, len is passed to copy_to_user(), which has a third parameter type of 'unsigned long', so it is widened and changes signs again. This excessive casting in combination with the KCSAN instrumentation causes LLVM to fail to eliminate the __bad_copy_from() call, failing the build.
The same issue occurs in rfcomm in functions rfcomm_sock_bind and rfcomm_sock_getsockopt_old.
Was this found by inspection or is there an actual instance of the same warning? For what it's worth, I haven't seen a warning from this file in any of my build tests.
We’ve seen build errors in rfcomm in the Chromium OS 4.14 kernel.
Change the type of len to size_t in both rfcomm_sock_bind and rfcomm_sock_getsockopt_old and replace min_t() with min().
Cc: stable@vger.kernel.org Fixes: 9bf4e919ccad ("Bluetooth: Fix type of len in {l2cap,sco}_sock_getsockopt_old()")
I am not sure that I totally agree with this Fixes tag since I did not have a warning to fix but I guess it makes sense to help with backporting.
It’s a shame there isn’t a Complements: or Improves: tag :)
On Wed, Oct 02, 2024 at 06:29:22PM +0200, Andrej Shadura wrote:
On Wed, Oct 02, 2024 at 04:12:17PM +0200, Andrej Shadura wrote:
Commit 9bf4e919ccad worked around an issue introduced after an innocuous optimisation change in LLVM main:
len is defined as an 'int' because it is assigned from '__user int *optlen'. However, it is clamped against the result of sizeof(), which has a type of 'size_t' ('unsigned long' for 64-bit platforms). This is done with min_t() because min() requires compatible types, which results in both len and the result of sizeof() being casted to 'unsigned int', meaning len changes signs and the result of sizeof() is truncated. From there, len is passed to copy_to_user(), which has a third parameter type of 'unsigned long', so it is widened and changes signs again. This excessive casting in combination with the KCSAN instrumentation causes LLVM to fail to eliminate the __bad_copy_from() call, failing the build.
The same issue occurs in rfcomm in functions rfcomm_sock_bind and rfcomm_sock_getsockopt_old.
Was this found by inspection or is there an actual instance of the same warning? For what it's worth, I haven't seen a warning from this file in any of my build tests.
We’ve seen build errors in rfcomm in the Chromium OS 4.14 kernel.
If there is any reason to send a v2 (since I don't think my nit below really warrants one at the moment), you may consider adding this information and the warning text that you see to the commit message for future travelers. I definitely have to go back to look at prior commits for understanding on current issues and I appreciate having easy things to look for when searching.
Is the warning reproducible on mainline with the same configuration? Just curious, I still think it is worth sending this through mainline even if there is no warning currently, since we cannot say that an potential future LLVM change could not make this show up there if not properly fixed. I am just wondering what my test coverage is missing :)
Change the type of len to size_t in both rfcomm_sock_bind and rfcomm_sock_getsockopt_old and replace min_t() with min().
Cc: stable@vger.kernel.org Fixes: 9bf4e919ccad ("Bluetooth: Fix type of len in {l2cap,sco}_sock_getsockopt_old()")
I am not sure that I totally agree with this Fixes tag since I did not have a warning to fix but I guess it makes sense to help with backporting.
It’s a shame there isn’t a Complements: or Improves: tag :)
Heh, who is to say we can't be the first? :)
There is some prior art with References: but eh, like I said above, I do not think it truly matters, since it should make it easier for the stable folks to apply it. That comment was more directed at how overloaded Fixes: has become to me that maybe it is worth considering expanding the type of tags for referring to previous commits but I am sure there would be push back in some form... No worries regardless!
Cheers, Nathan
Hi Andrej,
kernel test robot noticed the following build errors:
[auto build test ERROR on bluetooth-next/master] [also build test ERROR on bluetooth/master linus/master v6.12-rc1 next-20241004] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrej-Shadura/Bluetooth-Fix-... base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master patch link: https://lore.kernel.org/r/20241002141217.663070-1-andrew.shadura%40collabora... patch subject: [PATCH] Bluetooth: Fix type of len in rfcomm_sock_{bind,getsockopt_old}() config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20241004/202410041637.iOIxEAQQ-lkp@i...) compiler: alpha-linux-gcc (GCC) 13.3.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241004/202410041637.iOIxEAQQ-lkp@i...)
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/202410041637.iOIxEAQQ-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from <command-line>: net/bluetooth/rfcomm/sock.c: In function 'rfcomm_sock_bind':
include/linux/compiler_types.h:510:45: error: call to '__compiletime_assert_602' declared with attribute error: min(sizeof(sa), addr_len) signedness error
510 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ include/linux/compiler_types.h:491:25: note: in definition of macro '__compiletime_assert' 491 | prefix ## suffix(); \ | ^~~~~~ include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert' 510 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ include/linux/minmax.h:100:9: note: in expansion of macro 'BUILD_BUG_ON_MSG' 100 | BUILD_BUG_ON_MSG(!__types_ok(x,y,ux,uy), \ | ^~~~~~~~~~~~~~~~ include/linux/minmax.h:105:9: note: in expansion of macro '__careful_cmp_once' 105 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) | ^~~~~~~~~~~~~~~~~~ include/linux/minmax.h:129:25: note: in expansion of macro '__careful_cmp' 129 | #define min(x, y) __careful_cmp(min, x, y) | ^~~~~~~~~~~~~ net/bluetooth/rfcomm/sock.c:339:15: note: in expansion of macro 'min' 339 | len = min(sizeof(sa), addr_len); | ^~~
vim +/__compiletime_assert_602 +510 include/linux/compiler_types.h
eb5c2d4b45e3d2 Will Deacon 2020-07-21 496 eb5c2d4b45e3d2 Will Deacon 2020-07-21 497 #define _compiletime_assert(condition, msg, prefix, suffix) \ eb5c2d4b45e3d2 Will Deacon 2020-07-21 498 __compiletime_assert(condition, msg, prefix, suffix) eb5c2d4b45e3d2 Will Deacon 2020-07-21 499 eb5c2d4b45e3d2 Will Deacon 2020-07-21 500 /** eb5c2d4b45e3d2 Will Deacon 2020-07-21 501 * compiletime_assert - break build and emit msg if condition is false eb5c2d4b45e3d2 Will Deacon 2020-07-21 502 * @condition: a compile-time constant condition to check eb5c2d4b45e3d2 Will Deacon 2020-07-21 503 * @msg: a message to emit if condition is false eb5c2d4b45e3d2 Will Deacon 2020-07-21 504 * eb5c2d4b45e3d2 Will Deacon 2020-07-21 505 * In tradition of POSIX assert, this macro will break the build if the eb5c2d4b45e3d2 Will Deacon 2020-07-21 506 * supplied condition is *false*, emitting the supplied error message if the eb5c2d4b45e3d2 Will Deacon 2020-07-21 507 * compiler has support to do so. eb5c2d4b45e3d2 Will Deacon 2020-07-21 508 */ eb5c2d4b45e3d2 Will Deacon 2020-07-21 509 #define compiletime_assert(condition, msg) \ eb5c2d4b45e3d2 Will Deacon 2020-07-21 @510 _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) eb5c2d4b45e3d2 Will Deacon 2020-07-21 511
Hi Andrej,
kernel test robot noticed the following build errors:
[auto build test ERROR on bluetooth-next/master] [also build test ERROR on bluetooth/master linus/master v6.12-rc1 next-20241004] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrej-Shadura/Bluetooth-Fix-... base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master patch link: https://lore.kernel.org/r/20241002141217.663070-1-andrew.shadura%40collabora... patch subject: [PATCH] Bluetooth: Fix type of len in rfcomm_sock_{bind,getsockopt_old}() config: arm-randconfig-001-20241004 (https://download.01.org/0day-ci/archive/20241004/202410042221.Phncg973-lkp@i...) compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project fef3566a25ff0e34fb87339ba5e13eca17cec00f) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241004/202410042221.Phncg973-lkp@i...)
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/202410042221.Phncg973-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from net/bluetooth/rfcomm/sock.c:32: In file included from include/net/bluetooth/bluetooth.h:30: In file included from include/net/sock.h:46: In file included from include/linux/netdevice.h:38: In file included from include/net/net_namespace.h:43: In file included from include/linux/skbuff.h:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:8: In file included from include/linux/cacheflush.h:5: In file included from arch/arm/include/asm/cacheflush.h:10: In file included from include/linux/mm.h:2232: include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 517 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~
net/bluetooth/rfcomm/sock.c:339:8: error: call to '__compiletime_assert_549' declared with 'error' attribute: min(sizeof(sa), addr_len) signedness error
339 | len = min(sizeof(sa), addr_len); | ^ include/linux/minmax.h:129:19: note: expanded from macro 'min' 129 | #define min(x, y) __careful_cmp(min, x, y) | ^ include/linux/minmax.h:105:2: note: expanded from macro '__careful_cmp' 105 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) | ^ include/linux/minmax.h:100:2: note: expanded from macro '__careful_cmp_once' 100 | BUILD_BUG_ON_MSG(!__types_ok(x,y,ux,uy), \ | ^ note: (skipping 2 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all) include/linux/compiler_types.h:498:2: note: expanded from macro '_compiletime_assert' 498 | __compiletime_assert(condition, msg, prefix, suffix) | ^ include/linux/compiler_types.h:491:4: note: expanded from macro '__compiletime_assert' 491 | prefix ## suffix(); \ | ^ <scratch space>:174:1: note: expanded from here 174 | __compiletime_assert_549 | ^ 1 warning and 1 error generated.
vim +339 net/bluetooth/rfcomm/sock.c
326 327 static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len) 328 { 329 struct sockaddr_rc sa; 330 struct sock *sk = sock->sk; 331 int err = 0; 332 size_t len; 333 334 if (!addr || addr_len < offsetofend(struct sockaddr, sa_family) || 335 addr->sa_family != AF_BLUETOOTH) 336 return -EINVAL; 337 338 memset(&sa, 0, sizeof(sa));
339 len = min(sizeof(sa), addr_len);
340 memcpy(&sa, addr, len); 341 342 BT_DBG("sk %p %pMR", sk, &sa.rc_bdaddr); 343 344 lock_sock(sk); 345 346 if (sk->sk_state != BT_OPEN) { 347 err = -EBADFD; 348 goto done; 349 } 350 351 if (sk->sk_type != SOCK_STREAM) { 352 err = -EINVAL; 353 goto done; 354 } 355 356 write_lock(&rfcomm_sk_list.lock); 357 358 if (sa.rc_channel && 359 __rfcomm_get_listen_sock_by_addr(sa.rc_channel, &sa.rc_bdaddr)) { 360 err = -EADDRINUSE; 361 } else { 362 /* Save source address */ 363 bacpy(&rfcomm_pi(sk)->src, &sa.rc_bdaddr); 364 rfcomm_pi(sk)->channel = sa.rc_channel; 365 sk->sk_state = BT_BOUND; 366 } 367 368 write_unlock(&rfcomm_sk_list.lock); 369 370 done: 371 release_sock(sk); 372 return err; 373 } 374
Hi Andrej,
On Wed, Oct 02, 2024 at 04:12:17PM +0200, Andrej Shadura wrote:
Change the type of len to size_t in both rfcomm_sock_bind and rfcomm_sock_getsockopt_old and replace min_t() with min().
rfcomm_sock_bind doesn't use copy_to_user, are you sure it has the same issue?
@@ -328,14 +328,15 @@ static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr { struct sockaddr_rc sa; struct sock *sk = sock->sk;
- int len, err = 0;
- int err = 0;
- size_t len;
if (!addr || addr_len < offsetofend(struct sockaddr, sa_family) || addr->sa_family != AF_BLUETOOTH) return -EINVAL; memset(&sa, 0, sizeof(sa));
- len = min_t(unsigned int, sizeof(sa), addr_len);
- len = min(sizeof(sa), addr_len); memcpy(&sa, addr, len);
BT_DBG("sk %p %pMR", sk, &sa.rc_bdaddr);
This change produces a compilation error around min expression, as "kernel test robot" notices below. And I think rfcomm_sock_bind shouldn't be touched at all, it doesn't use copy_to_user and doesn't produce compile errors with latest Clang.
@@ -729,7 +730,8 @@ static int rfcomm_sock_getsockopt_old(struct socket *sock, int optname, char __u struct sock *l2cap_sk; struct l2cap_conn *conn; struct rfcomm_conninfo cinfo;
- int len, err = 0;
- int err = 0;
- size_t len; u32 opt;
BT_DBG("sk %p", sk); @@ -783,7 +785,7 @@ static int rfcomm_sock_getsockopt_old(struct socket *sock, int optname, char __u cinfo.hci_handle = conn->hcon->handle; memcpy(cinfo.dev_class, conn->hcon->dev_class, 3);
len = min_t(unsigned int, len, sizeof(cinfo));
len = min(len, sizeof(cinfo));
This looks ok. But there is the same pattern in rfcomm_sock_getsockopt (without old prefix) and it also uses copy_to_user and produces compile error with latest Clang.
Could you remove rfcomm_sock_bind patch and apply it to rfcomm_sock_getsockopt instead? Or I can send my version of the patch: we've encountered the same compile errors in rfcomm_sock_getsockopt and rfcomm_sock_getsockopt_old after updating Clang and would like to get it fixed.
linux-stable-mirror@lists.linaro.org
-
Aleksei Vetrov -
Andrej Shadura -
kernel test robot -
Nathan Chancellor