On Wed, Dec 04, 2024 at 03:30:13PM +0530, Nikunj A. Dadhania wrote:
> The above ones I have retained old code.
Right.
> GFP_KERNEL_ACCOUNT allocation are accounted in kmemcg and the below note from[1]
> ----------------------------------------------------------------------------
> Untrusted allocations triggered from userspace should be a subject of kmem
> accounting and must have __GFP_ACCOUNT bit set. There is the handy
> GFP_KERNEL_ACCOUNT shortcut for GFP_KERNEL allocations that should be accounted.
> ----------------------------------------------------------------------------
Interesting.
> For mdesc, I had kept it similar to snp_dev allocation, that is why it is
> having GFP_KERNEL.
>
> snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL);
> if (!snp_dev)
> - goto e_unmap;
> -
> - mdesc = devm_kzalloc(&pdev->dev, sizeof(struct snp_msg_desc), GFP_KERNEL);
>
> Let me know if mdesc allocation need to be GFP_KERNEL_ACCOUNT.
Let's audit that thing:
* snp_init_crypto - not really untrusted allocation. It is on the driver probe
path.
* get_report - I don't think so:
/*
* The intermediate response buffer is used while decrypting the
* response payload. Make sure that it has enough space to cover the
* authtag.
*/
resp_len = sizeof(report_resp->data) + mdesc->ctx->authsize;
report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT);
That resp_len is limited and that's on the guest_ioctl path which cannot
happen concurrently?
* get_ext_report - ditto
* alloc_shared_pages - all the allocations are limited but I guess that could
remain _ACCOUNT as a measure for future robustness.
And that was it.
So AFAICT, only one use case is semi-valid.
So maybe we should convert those remaining ones to boring GFP_KERNEL...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
