On 4/19/20 12:58 AM, Christoph Hellwig wrote:
On Sat, Apr 18, 2020 at 08:40:20AM -0700, Bart Van Assche wrote:
This can have a sideeffect not only bdi->dev_name will be truncated to 64
chars (which generally doesn't matter) but possibly also kobject name will
be truncated in the same way. Which may have user visible effects. E.g.
for fs/vboxsf 64 chars need not be enough. So shouldn't we rather do it the
other way around - i.e., let device_create_vargs() create the device name
and then copy to bdi->dev_name whatever fits?
How about using kvasprintf() instead of vsnprintf()?
That is what v1 did, see the thread in response to that on why it isn't
a good idea.
Are you perhaps referring to patch "[PATCH 3/8] bdi: add a ->dev_name
field to struct backing_dev_info"
(https://lore.kernel.org/linux-block/20200416071519.807660-4-hch@xxxxxx/)
and also to the replies to that patch? This is what I found in the
replies: "When driver try to to re-register bdi but without
release_bdi(), the old dev_name will be cover directly by the newer in
bdi_register_va(). So, I am not sure whether it can cause memory leak
for bdi->dev_name."
Has it been considered to avoid that leak by freeing bdi->dev_name from
unregister_bdi(), e.g. as follows?
void bdi_unregister(struct backing_dev_info *bdi)
{
+ char *dev_name;
/* make sure nobody finds us on the bdi_list anymore */
bdi_remove_from_list(bdi);
wb_shutdown(&bdi->wb);
cgwb_bdi_unregister(bdi);
if (bdi->dev) {
bdi_debug_unregister(bdi);
device_unregister(bdi->dev);
bdi->dev = NULL;
+ dev_name = bdi->dev_name;
+ bdi->dev_name = "(unregistered)";
+ kfree(dev_name);
}
if (bdi->owner) {
put_device(bdi->owner);
bdi->owner = NULL;
}
}
Thanks,
Bart.
