Re: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block

Re: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block

[Randy Dunlap]

On 8/21/20 9:25 AM, Konstantin Komarov wrote:


> +/* O:BAG:BAD:(A;OICI;FA;;;WD) */

What is that notation, please?

> +const u8 s_dir_security[] __aligned(8) = {
> +	0x01, 0x00, 0x04, 0x80, 0x30, 0x00, 0x00, 0x00, 0x40, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x14, 0x00, 0x00, 0x00, 0x02, 0x00, 0x1C, 0x00,
> +	0x01, 0x00, 0x00, 0x00, 0x00, 0x03, 0x14, 0x00, 0xFF, 0x01, 0x1F, 0x00,
> +	0x01, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
> +	0x01, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x20, 0x00, 0x00, 0x00,
> +	0x20, 0x02, 0x00, 0x00, 0x01, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05,
> +	0x20, 0x00, 0x00, 0x00, 0x20, 0x02, 0x00, 0x00,
> +};



> +
> +	if (0x10000 * sizeof(short) != inode->i_size) {
> +		err = -EINVAL;
> +		goto out;
> +	}

Please put constants on the right side of compares.


> +MODULE_AUTHOR("Konstantin   Komarov");

Drop one space in the name.


thanks.
-- 
~Randy

RE: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block

[Konstantin Komarov]

From: Randy Dunlap <rdunlap@infradead.org>
Sent: Friday, August 21, 2020 8:36 PM
> On 8/21/20 9:25 AM, Konstantin Komarov wrote:
> 
> 
> > +/* O:BAG:BAD:(A;OICI;FA;;;WD) */
> 
> What is that notation, please?
> 

Apologies. It's MS's SSDL. We will have it explained a bit more in V3.

> > +const u8 s_dir_security[] __aligned(8) = {
[]
> 
> 
> > +MODULE_AUTHOR("Konstantin   Komarov");
> 
> Drop one space in the name.
> 

Done, will be posted with V3.

> 
> thanks.
> --
> ~Randy

Re: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block

[Joe Perches]

On Fri, 2020-08-21 at 16:25 +0000, Konstantin Komarov wrote:
> Initialization of super block for fs/ntfs3
[]
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
[]
> +
> +/**
> + * ntfs_trace() - print preformated ntfs specific messages.
> + */
> +void __ntfs_trace(const struct super_block *sb, const char *level,
> +		  const char *fmt, ...)

This is a printk mechanism.

I suggest renaming this __ntfs_trace function to ntfs_printk
as there is a naming expectation conflict with the tracing
subsystem.

> +{
> +	struct va_format vaf;
> +	va_list args;
> +
> +	va_start(args, fmt);
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +	if (!sb)
> +		printk("%sntfs3: %pV", level, &vaf);
> +	else
> +		printk("%sntfs3: %s: %pV", level, sb->s_id, &vaf);
> +	va_end(args);
> +}

Also it would be rather smaller overall object code to
change the macros and uses to embed the KERN_<LEVEL> into
the format and remove the const char *level argument.

Use printk_get_level to retrieve the level from the format.

see fs/f2fs/super.c for an example.

This could be something like the below with a '\n' addition
to the format string to ensure that messages are properly
terminated and cannot be interleaved by other subsystems
content that might be in another simultaneously running
thread starting with KERN_CONT.

void ntfs_printk(const struct super_block *sb, const char *fmt, ...)
{
	struct va_format vaf;
	va_list args;
	int level;

	va_start(args, fmt);

	level = printk_get_level(fmt);
	vaf.fmt = printk_skip_level(fmt);
	vaf.va = &args;
	if (!sb)
		printk("%c%cntfs3: %pV\n",
		       KERN_SOH_ASCII, level, &vaf);
	else
		printk("%c%cntfs3: %s: %pV\n",
		       KERN_SOH_ASCII, level, sbi->sb->s_id, &vaf);

	va_end(args);
}

> +
> +/* prints info about inode using dentry case if */
> +void __ntfs_inode_trace(struct inode *inode, const char *level, const char *fmt,

ntfs_inode_printk

> +			...)
> +{
> +	struct super_block *sb = inode->i_sb;
> +	ntfs_sb_info *sbi = sb->s_fs_info;
> +	struct dentry *dentry;
> +	const char *name = "?";
> +	char buf[48];
> +	va_list args;
> +	struct va_format vaf;
> +
> +	if (!__ratelimit(&sbi->ratelimit))
> +		return;
> +
> +	dentry = d_find_alias(inode);
> +	if (dentry) {
> +		spin_lock(&dentry->d_lock);
> +		name = (const char *)dentry->d_name.name;
> +	} else {
> +		snprintf(buf, sizeof(buf), "r=%lx", inode->i_ino);
> +		name = buf;
> +	}
> +
> +	va_start(args, fmt);
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +	printk("%s%s on %s: %pV", level, name, sb->s_id, &vaf);
> +	va_end(args);
> +
> +	if (dentry) {
> +		spin_unlock(&dentry->d_lock);
> +		dput(dentry);
> +	}
> +}

Remove level and use printk_get_level as above.
Format string should use '\n' termination here too.

RE: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block

[Konstantin Komarov]

From: Joe Perches <joe@perches.com>
Sent: Friday, August 21, 2020 10:39 PM
> 
> On Fri, 2020-08-21 at 16:25 +0000, Konstantin Komarov wrote:
> > Initialization of super block for fs/ntfs3
> []
> > diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> []
> > +
> > +/**
> > + * ntfs_trace() - print preformated ntfs specific messages.
> > + */
> > +void __ntfs_trace(const struct super_block *sb, const char *level,
> > +		  const char *fmt, ...)
> 
> This is a printk mechanism.
> 
> I suggest renaming this __ntfs_trace function to ntfs_printk
> as there is a naming expectation conflict with the tracing
> subsystem.
> 
> > +{
[]
> > +	else
> > +		printk("%sntfs3: %s: %pV", level, sb->s_id, &vaf);
> > +	va_end(args);
> > +}
> 
> Also it would be rather smaller overall object code to
> change the macros and uses to embed the KERN_<LEVEL> into
> the format and remove the const char *level argument.
> 
> Use printk_get_level to retrieve the level from the format.
> 
> see fs/f2fs/super.c for an example.
> 
> This could be something like the below with a '\n' addition
> to the format string to ensure that messages are properly
> terminated and cannot be interleaved by other subsystems
> content that might be in another simultaneously running
> thread starting with KERN_CONT.
> 
> void ntfs_printk(const struct super_block *sb, const char *fmt, ...)
> {
> 	struct va_format vaf;
> 	va_list args;
> 	int level;
> 
> 	va_start(args, fmt);
> 
> 	level = printk_get_level(fmt);
> 	vaf.fmt = printk_skip_level(fmt);
> 	vaf.va = &args;
> 	if (!sb)
> 		printk("%c%cntfs3: %pV\n",
> 		       KERN_SOH_ASCII, level, &vaf);
> 	else
> 		printk("%c%cntfs3: %s: %pV\n",
> 		       KERN_SOH_ASCII, level, sbi->sb->s_id, &vaf);
> 
> 	va_end(args);
> }
> 
> > +
> > +/* prints info about inode using dentry case if */
> > +void __ntfs_inode_trace(struct inode *inode, const char *level, const char *fmt,
> 
> ntfs_inode_printk
> 
> > +			...)
> > +{
> > +	struct super_block *sb = inode->i_sb;
> > +	ntfs_sb_info *sbi = sb->s_fs_info;
> > +	struct dentry *dentry;
> > +	const char *name = "?";
> > +	char buf[48];
> > +	va_list args;
> > +	struct va_format vaf;
> > +
> > +	if (!__ratelimit(&sbi->ratelimit))
> > +		return;
> > +
> > +	dentry = d_find_alias(inode);
> > +	if (dentry) {
> > +		spin_lock(&dentry->d_lock);
> > +		name = (const char *)dentry->d_name.name;
> > +	} else {
> > +		snprintf(buf, sizeof(buf), "r=%lx", inode->i_ino);
> > +		name = buf;
> > +	}
> > +
> > +	va_start(args, fmt);
> > +	vaf.fmt = fmt;
> > +	vaf.va = &args;
> > +	printk("%s%s on %s: %pV", level, name, sb->s_id, &vaf);
> > +	va_end(args);
> > +
> > +	if (dentry) {
> > +		spin_unlock(&dentry->d_lock);
> > +		dput(dentry);
> > +	}
> > +}
> 
> Remove level and use printk_get_level as above.
> Format string should use '\n' termination here too.
> 

Thanks for pointing this out and for your effort with the patch, Joe. We will rework logging in V3 so that it's more compliant with Kernel's approach.

Re: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block

[Pali Rohár]

On Friday 21 August 2020 16:25:03 Konstantin Komarov wrote:
> +		case Opt_nls:
> +			match_strlcpy(nls_name, &args[0], sizeof(nls_name));
> +			break;
> +
> +		/* unknown option */
> +		default:
> +			if (!silent)
> +				ntfs_error(
> +					sb,
> +					"Unrecognized mount option \"%s\" or missing value",
> +					p);
> +			//return -EINVAL;
> +		}
> +	}
> +
> +out:
> +	if (nls_name[0]) {
> +		sbi->nls = load_nls(nls_name);
> +		if (!sbi->nls) {
> +			/* critical ?*/
> +			ntfs_error(sb, "failed to load \"%s\"\n", nls_name);
> +			//return -EINVAL;

Well, I think it is a fatal error if user supplied NLS encoding cannot
be loaded. If user via mount parameter specify that wants encoding XYZ
and kernel loads different (e.g. default one) then userspace would be
confused as it would expect encoding XYZ.

> +		}
> +	}
> +
> +	if (!sbi->nls) {
> +		sbi->nls = load_nls_default();
> +		if (!sbi->nls) {
> +			/* critical */
> +			ntfs_error(sb, "failed to load default nls");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}

RE: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block

[Konstantin Komarov]

From: Pali Rohár <pali@kernel.org>
Sent: Sunday, August 23, 2020 12:55 PM
> 
> On Friday 21 August 2020 16:25:03 Konstantin Komarov wrote:
> > +		case Opt_nls:
> > +			match_strlcpy(nls_name, &args[0], sizeof(nls_name));
> > +			break;
> > +
> > +		/* unknown option */
> > +		default:
> > +			if (!silent)
> > +				ntfs_error(
> > +					sb,
> > +					"Unrecognized mount option \"%s\" or missing value",
> > +					p);
> > +			//return -EINVAL;
> > +		}
> > +	}
> > +
> > +out:
> > +	if (nls_name[0]) {
> > +		sbi->nls = load_nls(nls_name);
> > +		if (!sbi->nls) {
> > +			/* critical ?*/
> > +			ntfs_error(sb, "failed to load \"%s\"\n", nls_name);
> > +			//return -EINVAL;
> 
> Well, I think it is a fatal error if user supplied NLS encoding cannot
> be loaded. If user via mount parameter specify that wants encoding XYZ
> and kernel loads different (e.g. default one) then userspace would be
> confused as it would expect encoding XYZ.
> 

Agreed. Will be fixed in V3.

> > +		}
> > +	}
> > +
> > +	if (!sbi->nls) {
> > +		sbi->nls = load_nls_default();
> > +		if (!sbi->nls) {
> > +			/* critical */
> > +			ntfs_error(sb, "failed to load default nls");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}