| Message ID | 20221025152420.198036-1-yhs@fb.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/core: Remove rcu attr for uverbs_api_ioctl_method.handler | expand |
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
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
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
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
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
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;
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(-)