| Message ID | 20241203152049.348806-1-lihaoyu499@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | f1d3334d604cc32db63f6e2b3283011e02294e54 |
| Headers | show |
| Series | net: wireless: sme: Initialize n_channels before accessing channels in cfg80211_conn_scan | expand |
On Tue, 2024-12-03 at 23:20 +0800, Haoyu Li wrote: > With the new __counted_by annocation in cfg80211_scan_request struct, > the "n_channels" struct member must be set before accessing the > "channels" array. Failing to do so will trigger a runtime warning > when enabling CONFIG_UBSAN_BOUNDS and CONFIG_FORTIFY_SOURCE. > > Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by") > > Signed-off-by: Haoyu Li <lihaoyu499@gmail.com> nit: there should be no newline between these My tolerance for this is going WAY down, it seems it's all just busy- work, and then everyone complains and I need to handle "urgent fixes" because of it etc. I'm having severe second thoughts about ever having accepted the __counted_by annotations, I think we should just revert it. Experiment failed, we found ... that the code is fine but constantly needs changes to make the checkers happy. johannes
On 03/12/24 09:25, Johannes Berg wrote: > On Tue, 2024-12-03 at 23:20 +0800, Haoyu Li wrote: >> With the new __counted_by annocation in cfg80211_scan_request struct, >> the "n_channels" struct member must be set before accessing the >> "channels" array. Failing to do so will trigger a runtime warning >> when enabling CONFIG_UBSAN_BOUNDS and CONFIG_FORTIFY_SOURCE. >> >> Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by") >> >> Signed-off-by: Haoyu Li <lihaoyu499@gmail.com> > > nit: there should be no newline between these > > My tolerance for this is going WAY down, it seems it's all just busy- > work, and then everyone complains and I need to handle "urgent fixes" > because of it etc. > > I'm having severe second thoughts about ever having accepted the > __counted_by annotations, I think we should just revert it. Experiment > failed, we found ... that the code is fine but constantly needs changes > to make the checkers happy. Thanks for taking these changes! - This is improving :) See below. "Right now, any addition of a counted_by annotation must also include an open-coded assignment of the counter variable after the allocation: p = alloc(p, array, how_many); p->counter = how_many; In order to avoid the tedious and error-prone work of manually adding the open-coded counted-by intializations everywhere in the Linux kernel, a new GCC builtin __builtin_counted_by_ref will be very useful to be added to help the adoption of the counted-by attribute. -- Built-in Function: TYPE __builtin_counted_by_ref (PTR) The built-in function '__builtin_counted_by_ref' checks whether the array object pointed by the pointer PTR has another object associated with it that represents the number of elements in the array object through the 'counted_by' attribute (i.e. the counted-by object). If so, returns a pointer to the corresponding counted-by object. If such counted-by object does not exist, returns a null pointer. This built-in function is only available in C for now. The argument PTR must be a pointer to an array. The TYPE of the returned value is a pointer type pointing to the corresponding type of the counted-by object or a void pointer type in case of a null pointer being returned. With this new builtin, the central allocator could be updated to: #define MAX(A, B) (A > B) ? (A) : (B) #define alloc(P, FAM, COUNT) ({ \ __auto_type __p = &(P); \ __auto_type __c = (COUNT); \ size_t __size = MAX (sizeof (*(*__p)),\ __builtin_offsetof (__typeof(*(*__p)),FAM) \ + sizeof (*((*__p)->FAM)) * __c); \ if ((*__p = kmalloc(__size))) { \ __auto_type ret = __builtin_counted_by_ref((*__p)->FAM); \ *_Generic(ret, void *: &(size_t){0}, default: ret) = __c; \ } \ }) And then structs can gain the counted_by attribute without needing additional open-coded counter assignments for each struct, and unannotated structs could still use the same allocator."[1] These changes have been merged already, and will likely be released in coming GCC 15. For Clang, see [2]. For the kmalloc-family changes, see [3] (a new version of this that includes the __builtin_counted_by_ref() update is coming soon). -Gustavo [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-October/665165.html [2] https://github.com/llvm/llvm-project/issues/99774 [3] https://lore.kernel.org/linux-hardening/20240822231324.make.666-kees@kernel.org/
On Tue, 2024-12-03 at 10:20 -0600, Gustavo A. R. Silva wrote: > > "Right now, any addition of a counted_by annotation must also > include an open-coded assignment of the counter variable after > the allocation: > > p = alloc(p, array, how_many); > p->counter = how_many; Not sure where you copied that from, but quite obviously Kees didn't follow that guidance in e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), otherwise we wouldn't have this patch. > -- Built-in Function: TYPE __builtin_counted_by_ref (PTR) Even with that though, we still have to actually implement it, and make sure we use struct_size everywhere when we allocate these things... In fact we probably need a new allocation function, not just struct_size, but rather kzalloc_struct_size(...) or so. Which e3eac9f32ec0 didn't do, and which anyway we still don't do e.g. in nl80211_trigger_scan() because we have multiple variable things in the allocation, so we *can't*. That therefore doesn't even help here. So that's not a very convincing argument. In a way moving again to "you need the newest unreleased compiler" makes it *worse*, not *better*? But of course if you do that now it'll basically mean again nobody is running it and you get to kick the can further down the road ... I still think it's a failed experiment. It didn't do any good here as far as I can tell, and we've spent a ton of time on it. johannes
diff --git a/net/wireless/sme.c b/net/wireless/sme.c index 431da30817a6..268171600087 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -83,6 +83,7 @@ static int cfg80211_conn_scan(struct wireless_dev *wdev) if (!request) return -ENOMEM; + request->n_channels = n_channels; if (wdev->conn->params.channel) { enum nl80211_band band = wdev->conn->params.channel->band; struct ieee80211_supported_band *sband =
With the new __counted_by annocation in cfg80211_scan_request struct, the "n_channels" struct member must be set before accessing the "channels" array. Failing to do so will trigger a runtime warning when enabling CONFIG_UBSAN_BOUNDS and CONFIG_FORTIFY_SOURCE. Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by") Signed-off-by: Haoyu Li <lihaoyu499@gmail.com> --- net/wireless/sme.c | 1 + 1 file changed, 1 insertion(+)