Xen blkfront: Fixed memory leak in xbd_connect()

[pratyush]

Summary

If gnttab_grant_foreign_access() fails for any of the indirection pages, the code breaks out of both the loops without freeing the local variable indirectpages, causing a memory leak.

[royger]

Do you also need to free cm_sg_refs and destroy the dmamap in case of failure?

sys/dev/xen/blkfront/blkfront.c

1346

I think it would be easier to just add the contigfree here, since you can avoid the indirectpages != NULL check.

[pratyush]

sys/dev/xen/blkfront/blkfront.c

1346

The contigmalloc(9) man page says:

The contigfree() function does not accept NULL as an address input, unlike free(9).

[royger]

sys/dev/xen/blkfront/blkfront.c

1346

Will blkfront work correctly if xbd_max_request_indirectpages > 0 and cm->cm_indirectionpages == NULL?

[pratyush]

sys/dev/xen/blkfront/blkfront.c

1346

It wouldn't.

I noticed another problem. There is no NULL check for contigmalloc() above. The man page seems to imply that contigmalloc() can fail even when M_NOWAIT is not passed.

The contigmalloc() function does not sleep waiting for memory resources
to be freed up, but instead actively reclaims pages before giving up.
However, unless M_NOWAIT is specified, it may select a page for reclamation
that must first be written to backing storage, causing it to sleep.

I think it would be better to add a NULL check there and remove it from here.

[pratyush]

sys/dev/xen/blkfront/blkfront.c

1346

I'm not sure what exactly the man page means. It could mean that unless M_NOWAIT is specified, it will evict a page from memory, and move it to disk (sleeping in the process). But it would still always succeed. In that case a NULL check is not needed.

Or it could mean that it is still possible for contigmalloc() to fail, but it will first try to evict a page to disk before giving up (and returning NULL).

Could you please confirm which one of these two is correct?

[pratyush]

Removed the NULL check.

[pratyush]

I asked on freebsd-hackers@ mailing list, and Conrad Meyer says that contigmalloc() can fail even when M_NOWAIT is not specified. Link to the email.

So there should be a NULL check after contigmalloc() above. Should I add that change in this same revision or create a different one?

[royger]

In D16136#343358, @pratyush wrote:

I asked on freebsd-hackers@ mailing list, and Conrad Meyer says that contigmalloc() can fail even when M_NOWAIT is not specified. Link to the email.

So there should be a NULL check after contigmalloc() above. Should I add that change in this same revision or create a different one?

Every memory allocation in xbd_connect uses m_NOWAIT, so I assume it's not safe to sleep in that function? If it's possible to sleep then you could use M_WAITOK which is guaranteed to never return NULL.

Also, could you fix your review requests so that the diff can be expanded? Right now it says "Context not available."

[pratyush]

The contigmalloc(9) man page says only 2 flags can be used: M_NOWAIT and M_ZERO. All other flags (if present) will be ignored. So M_WAITOK can not be used.

Since contigmalloc() above only specifies M_ZERO, it can sleep, but that does not guarantee success (as confirmed by Conrad Meyer in the email I linked in the previous comment). contigmalloc(9) works a little differently from malloc(9) in this regard. Since every other malloc in xbd_connect() specifies M_NOWAIT, I think that M_NOWAIT should be specified to contigmalloc as well, along with M_ZERO. This might be a simple mistake from the person who wrote the code.

Also, I have no idea why my diffs can't be expanded. I simply run git diff and then upload it here. Should I do something differently?

[pratyush]

Updated the diff to add flag M_NOWAIT to the contigmalloc() used to allocate the indirection pages. Also, added a NULL check there in case contigmalloc fails.

PS: I figured out how to show context. I just had to add the option -U9999 to git diff, to increase the number of context lines from the default 3 to more than the length of the file.

[royger]

In D16136#343373, @pratyush wrote:

Also, I have no idea why my diffs can't be expanded. I simply run git diff and then upload it here. Should I do something differently?

I think it will work better if you use the arc command line tool, see: https://wiki.freebsd.org/Phabricator

[royger]

The code is OK, but the coding style is not. I had to fix the lines to be < 80 cols and adjust the indentation.

[pratyush]

Probably a difference in the tab width. I don't get why there is indentation with both tabs and spaces in a single line. The entire blkfront code is like that. Some places its all tabs, some places it is a mix of tabs and spaces for indentation. Its weird and confusing.

[royger]

In D16136#350352, @pratyush wrote:

Probably a difference in the tab width. I don't get why there is indentation with both tabs and spaces in a single line. The entire blkfront code is like that. Some places its all tabs, some places it is a mix of tabs and spaces for indentation. Its weird and confusing.

style(9) details when you should use tabs or spaces:

"Indentation is an 8 character tab. Second level indents are four spaces. If you have to wrap a long statement, put the operator at the end of the line."

So if you have a function that has a long list of arguments it should be split like:

foobar(a, b, c,
    d);

[pratyush]

Committed by Roger as rS336896