On Mon, Jul 31, 2023 at 4:18 PM Dan Carpenter dan.carpenter@linaro.org wrote:

Hello Domenico Cerasuolo,

The patch 7c967f267b1d: "selftests: cgroup: add test_zswap with no kmem bypass test" from Jun 21, 2023 (linux-next), leads to the following Smatch static checker warning:

    ./tools/testing/selftests/cgroup/test_zswap.c:228 test_no_kmem_bypass()
    error: uninitialized symbol 'test_group'.

./tools/testing/selftests/cgroup/test_zswap.c 148 static int test_no_kmem_bypass(const char *root) 149 { 150 size_t min_free_kb_high, min_free_kb_low, min_free_kb_original; 151 struct no_kmem_bypass_child_args *values; 152 size_t trigger_allocation_size; 153 int wait_child_iteration = 0; 154 long stored_pages_threshold; 155 struct sysinfo sys_info; 156 int ret = KSFT_FAIL; 157 int child_status; 158 char *test_group; 159 pid_t child_pid; 160 161 /* Read sys info and compute test values accordingly */ 162 if (sysinfo(&sys_info) != 0) 163 return KSFT_FAIL; 164 if (sys_info.totalram > 5000000000) 165 return KSFT_SKIP; 166 values = mmap(0, sizeof(struct no_kmem_bypass_child_args), PROT_READ | 167 PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); 168 if (values == MAP_FAILED) 169 return KSFT_FAIL; 170 if (read_min_free_kb(&min_free_kb_original)) 171 return KSFT_FAIL; 172 min_free_kb_high = sys_info.totalram / 2000; 173 min_free_kb_low = sys_info.totalram / 500000; 174 values->target_alloc_bytes = (sys_info.totalram - min_free_kb_high * 1000) + 175 sys_info.totalram * 5 / 100; 176 stored_pages_threshold = sys_info.totalram / 5 / 4096; 177 trigger_allocation_size = sys_info.totalram / 20; 178 179 /* Set up test memcg */ 180 if (cg_write(root, "cgroup.subtree_control", "+memory")) 181 goto out;

test_group is not initialized here. Undoing stuff which hasn't been done is the canonical bug with "goto out;" labels.

True, this goto out and next one should just be return.

182         test_group = cg_name(root, "kmem_bypass_test");
183         if (!test_group)
184                 goto out;
185
186         /* Spawn memcg child and wait for it to allocate */
187         set_min_free_kb(min_free_kb_low);
188         if (cg_create(test_group))

test_group is allocated but not created. Is it okay to call destroy on this path?

It is, cg_destroy() just returns 0 if the path doesn't exist.

189                 goto out;
190         values->child_allocated = false;
191         child_pid = cg_run_nowait(test_group, no_kmem_bypass_child, values);
192         if (child_pid < 0)
193                 goto out;
194         while (!values->child_allocated && wait_child_iteration++ < 10000)
195                 usleep(1000);
196
197         /* Try to wakeup kswapd and let it push child memory to zswap */
198         set_min_free_kb(min_free_kb_high);
199         for (int i = 0; i < 20; i++) {
200                 size_t stored_pages;
201                 char *trigger_allocation = malloc(trigger_allocation_size);
202
203                 if (!trigger_allocation)
204                         break;
205                 for (int i = 0; i < trigger_allocation_size; i += 4095)
206                         trigger_allocation[i] = 'b';
207                 usleep(100000);
208                 free(trigger_allocation);
209                 if (get_zswap_stored_pages(&stored_pages))
210                         break;
211                 if (stored_pages < 0)
212                         break;
213                 /* If memory was pushed to zswap, verify it belongs to memcg */
214                 if (stored_pages > stored_pages_threshold) {
215                         int zswapped = cg_read_key_long(test_group, "memory.stat", "zswapped ");
216                         int delta = stored_pages * 4096 - zswapped;
217                         int result_ok = delta < stored_pages * 4096 / 4;
218
219                         ret = result_ok ? KSFT_PASS : KSFT_FAIL;
220                         break;
221                 }
222         }
223
224         kill(child_pid, SIGTERM);
225         waitpid(child_pid, &child_status, 0);
226 out:
227         set_min_free_kb(min_free_kb_original);

--> 228 cg_destroy(test_group); 229 free(test_group); 230 return ret; 231 }

regards, dan carpenter

Thanks again, will fix this too!

Domenico