* [PATCH] RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler
@ 2022-10-25 15:24 Yonghong Song
2022-10-25 16:00 ` Jason Gunthorpe
2022-10-25 23:33 ` kernel test robot
0 siblings, 2 replies; 6+ messages in thread
From: Yonghong Song @ 2022-10-25 15:24 UTC (permalink / raw)
To: Jason Gunthorpe, Leon Romanovsky, linux-rdma; +Cc: Paul E . McKenney
The current uverbs_api_ioctl_method definition:
struct uverbs_api_ioctl_method {
int(__rcu *handler)(struct uverbs_attr_bundle *attrs);
DECLARE_BITMAP(attr_mandatory, UVERBS_API_ATTR_BKEY_LEN);
...
};
The struct member 'handler' is marked with __rcu. But unless
the function body pointed by 'handler' is changing (e.g., jited)
during runtime, there is no need with __rcu.
I discovered this issue when I tried to define __rcu with
__attribute__((btf_type_tag("rcu"))). See [1] for an example
how __user is handled in a similar way.
clang crashed with __attribute__((btf_type_tag("rcu"))) since it
does not support such a pattern of btf_type_tag for a function
pointer. Removing __rcu attr for uverbs_api_ioctl_method.handler
allows building kernel successfully with __attribute__((btf_type_tag("rcu"))).
Since __rcu is not really needed in uverbs_api_ioctl_method.handler,
I suggest we remove it.
[1] https://lore.kernel.org/r/20220127154600.652613-1-yhs@fb.com
Signed-off-by: Yonghong Song <yhs@fb.com>
---
drivers/infiniband/core/rdma_core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/core/rdma_core.h b/drivers/infiniband/core/rdma_core.h
index 33706dad6c0f..633a241d355a 100644
--- a/drivers/infiniband/core/rdma_core.h
+++ b/drivers/infiniband/core/rdma_core.h
@@ -85,7 +85,7 @@ struct ib_udata *uverbs_get_cleared_udata(struct uverbs_attr_bundle *attrs);
*/
struct uverbs_api_ioctl_method {
- int(__rcu *handler)(struct uverbs_attr_bundle *attrs);
+ int(*handler)(struct uverbs_attr_bundle *attrs);
DECLARE_BITMAP(attr_mandatory, UVERBS_API_ATTR_BKEY_LEN);
u16 bundle_size;
u8 use_stack:1;
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler
2022-10-25 15:24 [PATCH] RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler Yonghong Song
@ 2022-10-25 16:00 ` Jason Gunthorpe
2022-10-25 16:29 ` Paul E. McKenney
2022-10-25 23:33 ` kernel test robot
1 sibling, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2022-10-25 16:00 UTC (permalink / raw)
To: Yonghong Song; +Cc: Leon Romanovsky, linux-rdma, Paul E . McKenney
On Tue, Oct 25, 2022 at 08:24:20AM -0700, Yonghong Song wrote:
> The current uverbs_api_ioctl_method definition:
> struct uverbs_api_ioctl_method {
> int(__rcu *handler)(struct uverbs_attr_bundle *attrs);
> DECLARE_BITMAP(attr_mandatory, UVERBS_API_ATTR_BKEY_LEN);
> ...
> };
> The struct member 'handler' is marked with __rcu. But unless
> the function body pointed by 'handler' is changing (e.g., jited)
> during runtime, there is no need with __rcu.
Huh? This is a sparse marker, it says that the pointer must always be
loaded with rcu_dereference
It has nothing to do with JIT, this patch is not correct
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler
2022-10-25 16:00 ` Jason Gunthorpe
@ 2022-10-25 16:29 ` Paul E. McKenney
2022-10-25 16:30 ` Jason Gunthorpe
0 siblings, 1 reply; 6+ messages in thread
From: Paul E. McKenney @ 2022-10-25 16:29 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Yonghong Song, Leon Romanovsky, linux-rdma
On Tue, Oct 25, 2022 at 01:00:02PM -0300, Jason Gunthorpe wrote:
> On Tue, Oct 25, 2022 at 08:24:20AM -0700, Yonghong Song wrote:
> > The current uverbs_api_ioctl_method definition:
> > struct uverbs_api_ioctl_method {
> > int(__rcu *handler)(struct uverbs_attr_bundle *attrs);
> > DECLARE_BITMAP(attr_mandatory, UVERBS_API_ATTR_BKEY_LEN);
> > ...
> > };
> > The struct member 'handler' is marked with __rcu. But unless
> > the function body pointed by 'handler' is changing (e.g., jited)
> > during runtime, there is no need with __rcu.
>
> Huh? This is a sparse marker, it says that the pointer must always be
> loaded with rcu_dereference
>
> It has nothing to do with JIT, this patch is not correct
OK, I will bite...
This is a pointer to a function. Given that this function's code is
generated at compile time, what sequence of changes is rcu_dereference()
protecting against?
Thanx, Paul
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler
2022-10-25 16:29 ` Paul E. McKenney
@ 2022-10-25 16:30 ` Jason Gunthorpe
2022-10-25 17:51 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2022-10-25 16:30 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: Yonghong Song, Leon Romanovsky, linux-rdma
On Tue, Oct 25, 2022 at 09:29:09AM -0700, Paul E. McKenney wrote:
> On Tue, Oct 25, 2022 at 01:00:02PM -0300, Jason Gunthorpe wrote:
> > On Tue, Oct 25, 2022 at 08:24:20AM -0700, Yonghong Song wrote:
> > > The current uverbs_api_ioctl_method definition:
> > > struct uverbs_api_ioctl_method {
> > > int(__rcu *handler)(struct uverbs_attr_bundle *attrs);
> > > DECLARE_BITMAP(attr_mandatory, UVERBS_API_ATTR_BKEY_LEN);
> > > ...
> > > };
> > > The struct member 'handler' is marked with __rcu. But unless
> > > the function body pointed by 'handler' is changing (e.g., jited)
> > > during runtime, there is no need with __rcu.
> >
> > Huh? This is a sparse marker, it says that the pointer must always be
> > loaded with rcu_dereference
> >
> > It has nothing to do with JIT, this patch is not correct
>
> OK, I will bite...
>
> This is a pointer to a function. Given that this function's code is
> generated at compile time, what sequence of changes is rcu_dereference()
> protecting against?
Module unload. We set the value to NULL and then synchronize_rcu
before unloading the code it points at.
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler
2022-10-25 16:30 ` Jason Gunthorpe
@ 2022-10-25 17:51 ` Yonghong Song
0 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2022-10-25 17:51 UTC (permalink / raw)
To: Jason Gunthorpe, Paul E. McKenney
Cc: Yonghong Song, Leon Romanovsky, linux-rdma
On 10/25/22 9:30 AM, Jason Gunthorpe wrote:
> On Tue, Oct 25, 2022 at 09:29:09AM -0700, Paul E. McKenney wrote:
>> On Tue, Oct 25, 2022 at 01:00:02PM -0300, Jason Gunthorpe wrote:
>>> On Tue, Oct 25, 2022 at 08:24:20AM -0700, Yonghong Song wrote:
>>>> The current uverbs_api_ioctl_method definition:
>>>> struct uverbs_api_ioctl_method {
>>>> int(__rcu *handler)(struct uverbs_attr_bundle *attrs);
>>>> DECLARE_BITMAP(attr_mandatory, UVERBS_API_ATTR_BKEY_LEN);
>>>> ...
>>>> };
>>>> The struct member 'handler' is marked with __rcu. But unless
>>>> the function body pointed by 'handler' is changing (e.g., jited)
>>>> during runtime, there is no need with __rcu.
>>>
>>> Huh? This is a sparse marker, it says that the pointer must always be
>>> loaded with rcu_dereference
>>>
>>> It has nothing to do with JIT, this patch is not correct
>>
>> OK, I will bite...
>>
>> This is a pointer to a function. Given that this function's code is
>> generated at compile time, what sequence of changes is rcu_dereference()
>> protecting against?
>
> Module unload. We set the value to NULL and then synchronize_rcu
> before unloading the code it points at.
Okay. Thanks for explanation. I forgot the module unload case.
I will look at compiler side then.
>
> Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler
2022-10-25 15:24 [PATCH] RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler Yonghong Song
2022-10-25 16:00 ` Jason Gunthorpe
@ 2022-10-25 23:33 ` kernel test robot
1 sibling, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-10-25 23:33 UTC (permalink / raw)
To: Yonghong Song, Jason Gunthorpe, Leon Romanovsky, linux-rdma
Cc: oe-kbuild-all, Paul E . McKenney
[-- Attachment #1: Type: text/plain, Size: 8211 bytes --]
Hi Yonghong,
I love your patch! Perhaps something to improve:
[auto build test WARNING on rdma/for-next]
[also build test WARNING on linus/master v6.1-rc2 next-20221025]
[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/Yonghong-Song/RDMA-core-Remove-rcu-attr-for-uverbs_api_ioctl_method-handler/20221025-232623
base: https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
patch link: https://lore.kernel.org/r/20221025152420.198036-1-yhs%40fb.com
patch subject: [PATCH] RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler
config: s390-randconfig-s031-20221025 (attached as .config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/e409cac2bfa16dc3530c72db03408b5c86ab8a93
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yonghong-Song/RDMA-core-Remove-rcu-attr-for-uverbs_api_ioctl_method-handler/20221025-232623
git checkout e409cac2bfa16dc3530c72db03408b5c86ab8a93
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=s390 SHELL=/bin/bash drivers/infiniband/core/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/infiniband/core/uverbs_uapi.c:122:17: sparse: sparse: incompatible types in comparison expression (different address spaces):
>> drivers/infiniband/core/uverbs_uapi.c:122:17: sparse: int ( [noderef] __rcu * )( ... )
>> drivers/infiniband/core/uverbs_uapi.c:122:17: sparse: int ( * )( ... )
drivers/infiniband/core/uverbs_uapi.c:698:33: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/infiniband/core/uverbs_uapi.c:698:33: sparse: int ( [noderef] __rcu * )( ... )
drivers/infiniband/core/uverbs_uapi.c:698:33: sparse: int ( * )( ... )
--
>> drivers/infiniband/core/uverbs_ioctl.c:431:19: sparse: sparse: incompatible types in comparison expression (different address spaces):
>> drivers/infiniband/core/uverbs_ioctl.c:431:19: sparse: int ( [noderef] __rcu * )( ... )
>> drivers/infiniband/core/uverbs_ioctl.c:431:19: sparse: int ( * )( ... )
vim +122 drivers/infiniband/core/uverbs_uapi.c
6884c6c4bd09fb Jason Gunthorpe 2018-11-12 96
9ed3e5f447723a Jason Gunthorpe 2018-08-09 97 static int uapi_merge_method(struct uverbs_api *uapi,
9ed3e5f447723a Jason Gunthorpe 2018-08-09 98 struct uverbs_api_object *obj_elm, u32 obj_key,
9ed3e5f447723a Jason Gunthorpe 2018-08-09 99 const struct uverbs_method_def *method,
9ed3e5f447723a Jason Gunthorpe 2018-08-09 100 bool is_driver)
9ed3e5f447723a Jason Gunthorpe 2018-08-09 101 {
9ed3e5f447723a Jason Gunthorpe 2018-08-09 102 u32 method_key = obj_key | uapi_key_ioctl_method(method->id);
9ed3e5f447723a Jason Gunthorpe 2018-08-09 103 struct uverbs_api_ioctl_method *method_elm;
9ed3e5f447723a Jason Gunthorpe 2018-08-09 104 unsigned int i;
c27f6aa8c9df7f Jason Gunthorpe 2018-11-12 105 bool exists;
9ed3e5f447723a Jason Gunthorpe 2018-08-09 106
9ed3e5f447723a Jason Gunthorpe 2018-08-09 107 if (!method->attrs)
9ed3e5f447723a Jason Gunthorpe 2018-08-09 108 return 0;
9ed3e5f447723a Jason Gunthorpe 2018-08-09 109
c27f6aa8c9df7f Jason Gunthorpe 2018-11-12 110 method_elm = uapi_add_get_elm(uapi, method_key, sizeof(*method_elm),
c27f6aa8c9df7f Jason Gunthorpe 2018-11-12 111 &exists);
c27f6aa8c9df7f Jason Gunthorpe 2018-11-12 112 if (IS_ERR(method_elm))
9ed3e5f447723a Jason Gunthorpe 2018-08-09 113 return PTR_ERR(method_elm);
c27f6aa8c9df7f Jason Gunthorpe 2018-11-12 114 if (exists) {
9ed3e5f447723a Jason Gunthorpe 2018-08-09 115 /*
9ed3e5f447723a Jason Gunthorpe 2018-08-09 116 * This occurs when a driver uses ADD_UVERBS_ATTRIBUTES_SIMPLE
9ed3e5f447723a Jason Gunthorpe 2018-08-09 117 */
9ed3e5f447723a Jason Gunthorpe 2018-08-09 118 if (WARN_ON(method->handler))
9ed3e5f447723a Jason Gunthorpe 2018-08-09 119 return -EINVAL;
9ed3e5f447723a Jason Gunthorpe 2018-08-09 120 } else {
9ed3e5f447723a Jason Gunthorpe 2018-08-09 121 WARN_ON(!method->handler);
9ed3e5f447723a Jason Gunthorpe 2018-08-09 @122 rcu_assign_pointer(method_elm->handler, method->handler);
9ed3e5f447723a Jason Gunthorpe 2018-08-09 123 if (method->handler != uverbs_destroy_def_handler)
9ed3e5f447723a Jason Gunthorpe 2018-08-09 124 method_elm->driver_method = is_driver;
9ed3e5f447723a Jason Gunthorpe 2018-08-09 125 }
9ed3e5f447723a Jason Gunthorpe 2018-08-09 126
9ed3e5f447723a Jason Gunthorpe 2018-08-09 127 for (i = 0; i != method->num_attrs; i++) {
9ed3e5f447723a Jason Gunthorpe 2018-08-09 128 const struct uverbs_attr_def *attr = (*method->attrs)[i];
9ed3e5f447723a Jason Gunthorpe 2018-08-09 129 struct uverbs_api_attr *attr_slot;
9ed3e5f447723a Jason Gunthorpe 2018-08-09 130
9ed3e5f447723a Jason Gunthorpe 2018-08-09 131 if (!attr)
9ed3e5f447723a Jason Gunthorpe 2018-08-09 132 continue;
9ed3e5f447723a Jason Gunthorpe 2018-08-09 133
9ed3e5f447723a Jason Gunthorpe 2018-08-09 134 /*
9ed3e5f447723a Jason Gunthorpe 2018-08-09 135 * ENUM_IN contains the 'ids' pointer to the driver's .rodata,
9ed3e5f447723a Jason Gunthorpe 2018-08-09 136 * so if it is specified by a driver then it always makes this
9ed3e5f447723a Jason Gunthorpe 2018-08-09 137 * into a driver method.
9ed3e5f447723a Jason Gunthorpe 2018-08-09 138 */
9ed3e5f447723a Jason Gunthorpe 2018-08-09 139 if (attr->attr.type == UVERBS_ATTR_TYPE_ENUM_IN)
9ed3e5f447723a Jason Gunthorpe 2018-08-09 140 method_elm->driver_method |= is_driver;
9ed3e5f447723a Jason Gunthorpe 2018-08-09 141
70cd20aed00f71 Guy Levi 2018-09-06 142 /*
70cd20aed00f71 Guy Levi 2018-09-06 143 * Like other uobject based things we only support a single
70cd20aed00f71 Guy Levi 2018-09-06 144 * uobject being NEW'd or DESTROY'd
70cd20aed00f71 Guy Levi 2018-09-06 145 */
70cd20aed00f71 Guy Levi 2018-09-06 146 if (attr->attr.type == UVERBS_ATTR_TYPE_IDRS_ARRAY) {
70cd20aed00f71 Guy Levi 2018-09-06 147 u8 access = attr->attr.u2.objs_arr.access;
70cd20aed00f71 Guy Levi 2018-09-06 148
70cd20aed00f71 Guy Levi 2018-09-06 149 if (WARN_ON(access == UVERBS_ACCESS_NEW ||
70cd20aed00f71 Guy Levi 2018-09-06 150 access == UVERBS_ACCESS_DESTROY))
70cd20aed00f71 Guy Levi 2018-09-06 151 return -EINVAL;
70cd20aed00f71 Guy Levi 2018-09-06 152 }
70cd20aed00f71 Guy Levi 2018-09-06 153
9ed3e5f447723a Jason Gunthorpe 2018-08-09 154 attr_slot =
9ed3e5f447723a Jason Gunthorpe 2018-08-09 155 uapi_add_elm(uapi, method_key | uapi_key_attr(attr->id),
9ed3e5f447723a Jason Gunthorpe 2018-08-09 156 sizeof(*attr_slot));
9ed3e5f447723a Jason Gunthorpe 2018-08-09 157 /* Attributes are not allowed to be modified by drivers */
9ed3e5f447723a Jason Gunthorpe 2018-08-09 158 if (IS_ERR(attr_slot))
9ed3e5f447723a Jason Gunthorpe 2018-08-09 159 return PTR_ERR(attr_slot);
9ed3e5f447723a Jason Gunthorpe 2018-08-09 160
9ed3e5f447723a Jason Gunthorpe 2018-08-09 161 attr_slot->spec = attr->attr;
9ed3e5f447723a Jason Gunthorpe 2018-08-09 162 }
9ed3e5f447723a Jason Gunthorpe 2018-08-09 163
9ed3e5f447723a Jason Gunthorpe 2018-08-09 164 return 0;
9ed3e5f447723a Jason Gunthorpe 2018-08-09 165 }
9ed3e5f447723a Jason Gunthorpe 2018-08-09 166
--
0-DAY CI Kernel Test Service
https://01.org/lkp
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41706 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-25 23:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-25 15:24 [PATCH] RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler Yonghong Song
2022-10-25 16:00 ` Jason Gunthorpe
2022-10-25 16:29 ` Paul E. McKenney
2022-10-25 16:30 ` Jason Gunthorpe
2022-10-25 17:51 ` Yonghong Song
2022-10-25 23:33 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox