linux-kernel - Re: AZFS file system proposal

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]

Date:	Mon, 7 Jul 2008 17:39:23 +0200
From:	Maxim Shchetynin <maxim@...ux.vnet.ibm.com>
To:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linuxppc-dev@...abs.org
Cc:	Arnd Bergmann <arnd@...db.de>
Subject: Re: AZFS file system proposal

Thank you Arnd for your comments. I have changed my patch accordinly (I will send it in a few minutes).

> > Subject: azfs: initial submit of azfs, a non-buffered filesystem
> 
> Please make the patch subject the actual subject of your email next time,
> and put the introductory text below the Signed-off-by: lines, separated
> by a "---" line. That will make the standard tools work without extra
> effort on my side. Also, please always Cc the person you want to merge
> the patch, in this case probably me.

Done.

> All other file systems are in separate directories, so it would be better
> to rename fs/azfs.c to fs/azfs/inode.c

Done.

> > +#define AZFS_SUPERBLOCK_FLAGS		MS_NOEXEC | \
> > +					MS_SYNCHRONOUS | \
> > +					MS_DIRSYNC | \
> > +					MS_ACTIVE
> 
> Why MS_NOEXEC? What happens on a remount if the user does not specifies
> -o remount,exec?

I also don't see any reason of keeping MS_NOEXEC - have just removed it.

> > +static unsigned long
> > +azfs_block_find(struct inode *inode, enum azfs_direction direction,
> > +		unsigned long from, unsigned long *size)
> > +{
> > ...
> > +}
> 
> This overloading of the return type to mean either a pointer or an offset
> on the block device is rather confusing. Why not just return the raw block_id
> before the last += and leave that part up to the caller?

Changed.

> > +		if (copy_to_user(target, (void*) pin, size)) {
> > +			rc = -EFAULT;
> > +			goto out;
> > +		}
> 
> Question to the powerpc folks: is copy_to_user safe for an __iomem source?
> Should there be two copies (memcpy_fromio and copy_to_user) instead?

I leave this question open.

> > +	page_prot = pgprot_val(vma->vm_page_prot);
> > +	page_prot |= (_PAGE_NO_CACHE | _PAGE_RW);
> > +	page_prot &= ~_PAGE_GUARDED;
> > +	vma->vm_page_prot = __pgprot(page_prot);
> 
> The pgprot modifications rely on powerpc specific flags, but the
> file system should not really need to be powerpc only.
> 
> The flags we want are more or less the same as PAGE_AGP, because
> both are I/O mapped memory that needs to be uncached but should
> not be guarded, for performance reasons.
> 
> Maybe we can introduce a new PAGE_IOMEM here that we can use
> in all places that need something like this. In spufs we need
> the same flags for the local store mappings.
> 
> I wouldn't hold up merging the file system for this problem, but
> until it is solved, the Kconfig entry should probably have
> a "depends on PPC".

Done.

-- 
Mit freundlichen Grüßen / met vriendelijke groeten / avec regards

    Maxim V. Shchetynin
    Linux Kernel Entwicklung
    IBM Deutschland Entwicklung GmbH
    Linux für Cell, Abteilung 3250
    Schönaicher Straße 220
    71032 Böblingen

Vorsitzender des Aufsichtsrats: Johann Weihen
Geschäftsführung: Herbert Kircher
Sitz der Gesellschaft: Böblingen
Registriergericht: Amtsgericht Stuttgart, HRB 243294

Fahr nur so schnell wie dein Schutzengel fliegen kann!
--
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/