| Message ID | 20230418225343.553806-7-sdf@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: handle another corner case in getsockopt | expand |
Hi Stanislav,
kernel test robot noticed the following build warnings:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Stanislav-Fomichev/bpf-Don-t-EFAULT-for-getsockopt-with-optval-NULL/20230419-065442
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20230418225343.553806-7-sdf%40google.com
patch subject: [PATCH bpf-next 6/6] bpf: Document EFAULT changes for sockopt
reproduce:
# https://github.com/intel-lab-lkp/linux/commit/789f0fbf25934464ae56e0022939fc77d4615d65
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Stanislav-Fomichev/bpf-Don-t-EFAULT-for-getsockopt-with-optval-NULL/20230419-065442
git checkout 789f0fbf25934464ae56e0022939fc77d4615d65
make menuconfig
# enable CONFIG_COMPILE_TEST, CONFIG_WARN_MISSING_DOCUMENTS, CONFIG_WARN_ABI_ERRORS
make htmldocs
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304200301.XukL6sTb-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> Documentation/bpf/prog_cgroup_sockopt.rst:115: WARNING: Unexpected indentation.
>> Documentation/bpf/prog_cgroup_sockopt.rst:111: WARNING: Inline literal start-string without end-string.
>> Documentation/bpf/prog_cgroup_sockopt.rst:111: WARNING: Inline emphasis start-string without end-string.
>> Documentation/bpf/prog_cgroup_sockopt.rst:121: WARNING: Block quote ends without a blank line; unexpected unindent.
>> Documentation/bpf/prog_cgroup_sockopt.rst:159: WARNING: Title level inconsistent:
vim +115 Documentation/bpf/prog_cgroup_sockopt.rst
110
> 111 ```
112 SEC("cgroup/getsockopt")
113 int getsockopt(struct bpf_sockopt *ctx)
114 {
> 115 /* Custom socket option. */
116 if (ctx->level == MY_SOL && ctx->optname == MY_OPTNAME) {
117 ctx->retval = 0;
118 optval[0] = ...;
119 ctx->optlen = 1;
120 return 1;
> 121 }
122
123 /* Modify kernel's socket option. */
124 if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
125 ctx->retval = 0;
126 optval[0] = ...;
127 ctx->optlen = 1;
128 return 1;
129 }
130
131 /* optval larger than PAGE_SIZE use kernel's buffer. */
132 if (ctx->optlen > 4096)
133 ctx->optlen = 0;
134
135 return 1;
136 }
137
138 SEC("cgroup/setsockopt")
139 int setsockopt(struct bpf_sockopt *ctx)
140 {
141 /* Custom socket option. */
142 if (ctx->level == MY_SOL && ctx->optname == MY_OPTNAME) {
143 /* do something */
144 ctx->optlen = -1;
145 return 1;
146 }
147
148 /* Modify kernel's socket option. */
149 if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
150 optval[0] = ...;
151 return 1;
152 }
153
154 /* optval larger than PAGE_SIZE use kernel's buffer. */
155 if (ctx->optlen > 4096)
156 ctx->optlen = 0;
157
158 return 1;
> 159 }
160 ```
161
On Wed, Apr 19, 2023 at 1:08 PM kernel test robot <lkp@intel.com> wrote: > > Hi Stanislav, > > kernel test robot noticed the following build warnings: Stupid me using ``` blocks instead of code-block:: c. Will address once I hear some feedback on the rest of the patches. > [auto build test WARNING on bpf-next/master] > > url: https://github.com/intel-lab-lkp/linux/commits/Stanislav-Fomichev/bpf-Don-t-EFAULT-for-getsockopt-with-optval-NULL/20230419-065442 > base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master > patch link: https://lore.kernel.org/r/20230418225343.553806-7-sdf%40google.com > patch subject: [PATCH bpf-next 6/6] bpf: Document EFAULT changes for sockopt > reproduce: > # https://github.com/intel-lab-lkp/linux/commit/789f0fbf25934464ae56e0022939fc77d4615d65 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Stanislav-Fomichev/bpf-Don-t-EFAULT-for-getsockopt-with-optval-NULL/20230419-065442 > git checkout 789f0fbf25934464ae56e0022939fc77d4615d65 > make menuconfig > # enable CONFIG_COMPILE_TEST, CONFIG_WARN_MISSING_DOCUMENTS, CONFIG_WARN_ABI_ERRORS > make htmldocs > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > | Link: https://lore.kernel.org/oe-kbuild-all/202304200301.XukL6sTb-lkp@intel.com/ > > All warnings (new ones prefixed by >>): > > >> Documentation/bpf/prog_cgroup_sockopt.rst:115: WARNING: Unexpected indentation. > >> Documentation/bpf/prog_cgroup_sockopt.rst:111: WARNING: Inline literal start-string without end-string. > >> Documentation/bpf/prog_cgroup_sockopt.rst:111: WARNING: Inline emphasis start-string without end-string. > >> Documentation/bpf/prog_cgroup_sockopt.rst:121: WARNING: Block quote ends without a blank line; unexpected unindent. > >> Documentation/bpf/prog_cgroup_sockopt.rst:159: WARNING: Title level inconsistent: > > vim +115 Documentation/bpf/prog_cgroup_sockopt.rst > > 110 > > 111 ``` > 112 SEC("cgroup/getsockopt") > 113 int getsockopt(struct bpf_sockopt *ctx) > 114 { > > 115 /* Custom socket option. */ > 116 if (ctx->level == MY_SOL && ctx->optname == MY_OPTNAME) { > 117 ctx->retval = 0; > 118 optval[0] = ...; > 119 ctx->optlen = 1; > 120 return 1; > > 121 } > 122 > 123 /* Modify kernel's socket option. */ > 124 if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) { > 125 ctx->retval = 0; > 126 optval[0] = ...; > 127 ctx->optlen = 1; > 128 return 1; > 129 } > 130 > 131 /* optval larger than PAGE_SIZE use kernel's buffer. */ > 132 if (ctx->optlen > 4096) > 133 ctx->optlen = 0; > 134 > 135 return 1; > 136 } > 137 > 138 SEC("cgroup/setsockopt") > 139 int setsockopt(struct bpf_sockopt *ctx) > 140 { > 141 /* Custom socket option. */ > 142 if (ctx->level == MY_SOL && ctx->optname == MY_OPTNAME) { > 143 /* do something */ > 144 ctx->optlen = -1; > 145 return 1; > 146 } > 147 > 148 /* Modify kernel's socket option. */ > 149 if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) { > 150 optval[0] = ...; > 151 return 1; > 152 } > 153 > 154 /* optval larger than PAGE_SIZE use kernel's buffer. */ > 155 if (ctx->optlen > 4096) > 156 ctx->optlen = 0; > 157 > 158 return 1; > > 159 } > 160 ``` > 161 > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests
diff --git a/Documentation/bpf/prog_cgroup_sockopt.rst b/Documentation/bpf/prog_cgroup_sockopt.rst index 172f957204bf..dcb8d4681257 100644 --- a/Documentation/bpf/prog_cgroup_sockopt.rst +++ b/Documentation/bpf/prog_cgroup_sockopt.rst @@ -29,7 +29,7 @@ chain finish (i.e. kernel ``setsockopt`` handling will *not* be executed). Note, that ``optlen`` can not be increased beyond the user-supplied value. It can only be decreased or set to -1. Any other value will -trigger ``EFAULT``. +ignore the BPF program's changes to ``optlen``/``optval``. Return Type ----------- @@ -45,13 +45,14 @@ sockopt. The BPF hook can observe ``optval``, ``optlen`` and ``retval`` if it's interested in whatever kernel has returned. BPF hook can override the values above, adjust ``optlen`` and reset ``retval`` to 0. If ``optlen`` has been increased above initial ``getsockopt`` value (i.e. userspace -buffer is too small), ``EFAULT`` is returned. +buffer is too small), BPF program's ``optlen`` and ``optval`` are +ignored. This hook has access to the cgroup and socket local storage. Note, that the only acceptable value to set to ``retval`` is 0 and the original value that the kernel returned. Any other value will trigger -``EFAULT``. +ignore BPF program's changes. Return Type ----------- @@ -98,10 +99,65 @@ When the ``optval`` is greater than the ``PAGE_SIZE``, the BPF program indicates that the kernel should use BPF's trimmed ``optval``. When the BPF program returns with the ``optlen`` greater than -``PAGE_SIZE``, the userspace will receive ``EFAULT`` errno. +``PAGE_SIZE``, the userspace will receive original kernel +buffers without any modifications that the BPF program might have +applied. Example ======= +Recommended way to handle BPF programs is as follows: + +``` +SEC("cgroup/getsockopt") +int getsockopt(struct bpf_sockopt *ctx) +{ + /* Custom socket option. */ + if (ctx->level == MY_SOL && ctx->optname == MY_OPTNAME) { + ctx->retval = 0; + optval[0] = ...; + ctx->optlen = 1; + return 1; + } + + /* Modify kernel's socket option. */ + if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) { + ctx->retval = 0; + optval[0] = ...; + ctx->optlen = 1; + return 1; + } + + /* optval larger than PAGE_SIZE use kernel's buffer. */ + if (ctx->optlen > 4096) + ctx->optlen = 0; + + return 1; +} + +SEC("cgroup/setsockopt") +int setsockopt(struct bpf_sockopt *ctx) +{ + /* Custom socket option. */ + if (ctx->level == MY_SOL && ctx->optname == MY_OPTNAME) { + /* do something */ + ctx->optlen = -1; + return 1; + } + + /* Modify kernel's socket option. */ + if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) { + optval[0] = ...; + return 1; + } + + /* optval larger than PAGE_SIZE use kernel's buffer. */ + if (ctx->optlen > 4096) + ctx->optlen = 0; + + return 1; +} +``` + See ``tools/testing/selftests/bpf/progs/sockopt_sk.c`` for an example of BPF program that handles socket options.
And add examples for how to correctly handle large optlens. This is less relevant now when we don't EFAULT anymore, but that's still the correct thing to do. Signed-off-by: Stanislav Fomichev <sdf@google.com> --- Documentation/bpf/prog_cgroup_sockopt.rst | 64 +++++++++++++++++++++-- 1 file changed, 60 insertions(+), 4 deletions(-)