linux-kernel - Re: [PATCH 6/8] exofs: super_operations and file_system

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: <20090215172443.GB18115@ioremap.net>
Date:	Sun, 15 Feb 2009 20:24:43 +0300
From:	Evgeniy Polyakov <zbr@...emap.net>
To:	Boaz Harrosh <bharrosh@...asas.com>
Cc:	Avishay Traeger <avishay@...il.com>, Jeff Garzik <jeff@...zik.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	open-osd <osd-dev@...n-osd.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	James Bottomley <James.Bottomley@...senPartnership.com>
Subject: Re: [PATCH 6/8] exofs: super_operations and file_system_type

Hi.

On Mon, Feb 09, 2009 at 03:25:53PM +0200, Boaz Harrosh (bharrosh@...asas.com) wrote:
> +static int parse_options(char *options, struct exofs_mountopt *opts)
> +{
> +	char *p;
> +	substring_t args[MAX_OPT_ARGS];
> +	int option;
> +	bool s_pid = false;
> +
> +	EXOFS_DBGMSG("parse_options %s\n", options);
> +	/* defaults */
> +	memset(opts, 0, sizeof(*opts));
> +	opts->timeout = BLK_DEFAULT_SG_TIMEOUT;
> +
> +	while ((p = strsep(&options, ",")) != NULL) {
> +		int token;
> +		char str[32];
> +
> +		if (!*p)
> +			continue;
> +
> +		token = match_token(p, tokens, args);
> +		switch (token) {
> +		case Opt_pid:
> +			if (0 == match_strlcpy(str, &args[0], sizeof(str)))
> +				return -EINVAL;
> +			opts->pid = simple_strtoull(str, NULL, 0);
> +			if (opts->pid < EXOFS_MIN_PID) {
> +				EXOFS_ERR("Partition ID must be >= %u",
> +					  EXOFS_MIN_PID);
> +				return -EINVAL;
> +			}
> +			s_pid = 1;
> +			break;
> +		case Opt_to:
> +			if (match_int(&args[0], &option))
> +				return -EINVAL;
> +			if (option <= 0) {
> +				EXOFS_ERR("Timout must be > 0");
> +				return -EINVAL;
> +			}
> +			opts->timeout = option * HZ;

Is it intentional to be a different timeouton systems with different HX
but the same mount option?

> +static struct inode *exofs_alloc_inode(struct super_block *sb)
> +{
> +	struct exofs_i_info *oi;
> +
> +	oi = kmem_cache_alloc(exofs_inode_cachep, GFP_KERNEL);

I'm curious if this should be GFP_NOFS or not?

> +	if (!oi)
> +		return NULL;
> +
> +	oi->vfs_inode.i_version = 1;
> +	return &oi->vfs_inode;
> +}

> +static void exofs_put_super(struct super_block *sb)
> +{
> +	int num_pend;
> +	struct exofs_sb_info *sbi = sb->s_fs_info;
> +
> +	/* make sure there are no pending commands */
> +	for (num_pend = atomic_read(&sbi->s_curr_pending); num_pend > 0;
> +	     num_pend = atomic_read(&sbi->s_curr_pending)) {

This rises a question. Let's check exofs_new_inode() for example (it is
a bad example, since inode can not be created when we already in the
put_super() callback, but still there are others), it increments
s_curr_pending way after inode was created, so is it possible that
some in-flight callback is about to be executed and its subsequent
s_curr_pending manipulation will not be detected by this loop?

Should s_curr_pending increment be audited all over the code to be
increased before the potential postponing command starts (which is not
the case in exofs_new_inode() above)?

> +		wait_queue_head_t wq;
> +		init_waitqueue_head(&wq);
> +		wait_event_timeout(wq,
> +				  (atomic_read(&sbi->s_curr_pending) == 0),
> +				  msecs_to_jiffies(100));
> +	}
> +
> +	osduld_put_device(sbi->s_dev);
> +	kfree(sb->s_fs_info);
> +	sb->s_fs_info = NULL;
> +}

-- 
	Evgeniy Polyakov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/