>
> The error handling in ksmbd_server_init() uses "one function to free everything style" which is
> impossible to audit and leads to several canonical bugs. When we free something that wasn't allocated
> it may be uninitialized, an error pointer, freed in a different function or we try freeing "foo->bar"
> when "foo" is a NULL pointer. And since the code is impossible to audit then it leads to memory leaks.
>
> In the ksmbd_server_init() function, every goto will lead to a crash because we have not allocated the
> work queue but we call
> ksmbd_workqueue_destroy() which tries to flush a NULL work queue.
> Another bug is if ksmbd_init_buffer_pools() fails then it leads to a double free because we free
> "work_cache" twice. A third type of bug is that we forgot to call ksmbd_release_inode_hash() so that
> is a resource leak.
>
> A better way to write error handling is for every function to clean up after itself and never leave
> things partially allocated. Then we can use "free the last successfully allocated resource" style.
> That way when someone is reading the code they can just track the last resource in their head and
> verify that the goto matches what they expect.
>
> In this patch I modified ksmbd_ipc_init() to clean up after itself and then I converted
> ksmbd_server_init() to use gotos to clean up.
>
> Fixes: cabcebc31de4 ("cifsd: introduce SMB3 kernel server")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
> v2: remove __exit annotation from ksmbd_release_inode_hash() as detected by the kbuild-bot
I will apply. Thanks for your patch!
