Messages in this thread Patch in this message | | | Date | Wed, 29 Apr 2020 04:26:37 -0700 | From | Christoph Hellwig <> | Subject | Re: [PATCH v3 4/6] blktrace: fix debugfs use after free |
| |
I can't say I'm a fan of all these long backtraces in commit logs..
> +static struct dentry *blk_debugfs_dir_register(const char *name) > +{ > + return debugfs_create_dir(name, blk_debugfs_root); > +}
I don't think we really need this helper.
> +void blk_part_debugfs_unregister(struct hd_struct *p) > +{ > + debugfs_remove_recursive(p->debugfs_dir); > + p->debugfs_dir = NULL; > +}
Why do we need to clear the pointer here?
> +#ifdef CONFIG_DEBUG_FS > + /* Currently only used by kernel/trace/blktrace.c */ > + struct dentry *debugfs_dir; > +#endif
Does that comment really add value?
> +static struct dentry *blk_trace_debugfs_dir(struct block_device *bdev, > + struct request_queue *q) > { > + struct hd_struct *p = NULL; > > + * Some drivers like scsi-generic use a NULL block device. For > + * other drivers when bdev != bdev->bd_contain we are doing a blktrace > + * on a parition, otherwise we know we are working on the whole > + * disk, and for that the request_queue already has its own debugfs_dir. > + * which we have been using for other things other than blktrace. > + */ > + if (bdev && bdev != bdev->bd_contains) > + p = bdev->bd_part; > > + if (p) > + return p->debugfs_dir; > + > + return q->debugfs_dir;
This could be simplified down to:
if (bdev && bdev != bdev->bd_contains) return bdev->bd_part->debugfs_dir; return q->debugfs_dir;
Given that bd_part is in __blkdev_get very near bd_contains.
Also given that this patch completely rewrites blk_trace_debugfs_dir is there any point in the previous patch?
> @@ -491,6 +500,7 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, > struct dentry *dir = NULL; > int ret; > > + > if (!buts->buf_size || !buts->buf_nr) > return -EINVAL; >
Spurious whitespace change.
| |