Messages in this thread | | | Date | Fri, 18 Feb 2022 09:46:11 +0100 | Subject | Re: PROBLEM: kasprintf WARN() seen during AMD XGBE debugfs creation on renaming race | From | Rasmus Villemoes <> |
| |
On 17/02/2022 15.39, Pighin, Anthony (Nokia - CA/Ottawa) wrote: > PROBLEM: Commit 8e2a2bfdb86ecb2421e3dd18d0fbbb42f2d943ad is being > triggered during debugfs entry creation in > ./drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c > > > > Seen in 5.4.153, 5.15.22, and 5.17-rc3 (ie. this bug is still present). >
Well, it's not really 8e2a2bfdb that is buggy, but the caller of kasprintf() that doesn't prevent a %s argument from changing midway through the call. In fact this simply shows that 8e2a2bfdb was justified.
In this case, since IFNAMSIZ is just 16, it would probably be simplest and easiest just to avoid kasprintf() and use a small stack buffer
- char *buf; + char buf[32];
- buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name); - if (!buf) - return; + snprintf(buf, sizeof(buf), "amd-xgbe-%s", pdata->netdev->name);
- kfree(buf);
It's still possible to get garbage in the output (a mix of the old and new name), of course.
But IIRC correctly, updating the netdev->name is done carefully in a way that there's always a nul-terminator, so snprintf() should not accidentally walk off the end of netdev->name [look at string() in vsprintf.c, it has been carefully rewritten to not walk the string twice]. And if not, there's really nothing snprintf() or kasprintf() could do about passing a racy netdev->name .
And while in there, the use of kasprintf() in xgbe_common_read() might as well be avoided, a 16 byte stack buffer is plenty, and removes the need for the 'if (!buf)' check and multiple kfree(buf) on the various return paths.
Rasmus
| |