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@xxxxxxxxxxxxxxx
> >> 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
