linux-kernel - Re: [PATCH v3 2/4] ocfs2: sysfile interfaces for online file check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160113233538.GL819@wotan.suse.de>
Date:	Wed, 13 Jan 2016 15:35:39 -0800
From:	Mark Fasheh <mfasheh@...e.de>
To:	Gang He <ghe@...e.com>
Cc:	rgoldwyn@...e.de, linux-kernel@...r.kernel.org,
	ocfs2-devel@....oracle.com, akpm@...ux-foundation.org
Subject: Re: [PATCH v3 2/4] ocfs2: sysfile interfaces for online file check

On Fri, Dec 25, 2015 at 03:16:17PM +0800, Gang He wrote:
> Implement online file check sysfile interfaces, e.g.
> how to create the related sysfile according to device name,
> how to display/handle file check request from the sysfile.
> 
> Signed-off-by: Gang He <ghe@...e.com>

Most of this looks good, I have two comments below. Also thank you for
redoing the interface to be more sysfs friendly.


> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
> new file mode 100644
> index 0000000..a83e4ba
> --- /dev/null
> +++ b/fs/ocfs2/filecheck.c
> @@ -0,0 +1,605 @@
> +static DEFINE_SPINLOCK(ocfs2_filecheck_sysfs_lock);
> +static LIST_HEAD(ocfs2_filecheck_sysfs_list);
> +
> +struct ocfs2_filecheck {
> +	struct list_head fc_head;	/* File check entry list head */
> +	spinlock_t fc_lock;
> +	unsigned int fc_max;	/* Maximum number of entry in list */

What is the point of fc_max? Only root can initiate file check so we need
not worry about a malicious user eating up our memory. That should let us
drop a bunch of the code below that is concerned with setting/reporting it.


> +	unsigned int fc_size;	/* Current entry count in list */
> +	unsigned int fc_done;	/* Finished entry count in list */
> +};
> +
> +struct ocfs2_filecheck_sysfs_entry {	/* sysfs entry per mounting */
> +	struct list_head fs_list;
> +	atomic_t fs_count;
> +	struct super_block *fs_sb;
> +	struct kset *fs_devicekset;
> +	struct kset *fs_fcheckkset;
> +	struct ocfs2_filecheck *fs_fcheck;
> +};
> +
> +#define OCFS2_FILECHECK_MAXSIZE		100
> +#define OCFS2_FILECHECK_MINSIZE		10
> +
> +/* File check operation type */
> +enum {
> +	OCFS2_FILECHECK_TYPE_CHK = 0,	/* Check a file(inode) */
> +	OCFS2_FILECHECK_TYPE_FIX,	/* Fix a file(inode) */
> +	OCFS2_FILECHECK_TYPE_SET = 100	/* Set entry list maximum size */
> +};
> +
> +struct ocfs2_filecheck_entry {
> +	struct list_head fe_list;
> +	unsigned long fe_ino;
> +	unsigned int fe_type;
> +	unsigned short fe_done:1;
> +	unsigned short fe_status:15;

I don't see the need to use a short here (or bitfield) for fc_status. IMHO
it is less error-prone if we just make it an int or unsigned int.


This is a bit off topic but I dream of the day when we can return errors
which userspace undestands but are outside the tiny range of 0-255  :)

Thanks,
	--Mark

--
Mark Fasheh