Re: [PATCH 01/17] mm: introduce MAP_SHARED_VALIDATE, a mechanism to safely define new mmap flags

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri 20-10-17 00:27:07, Christoph Hellwig wrote:
> >  	if (file) {
> >  		struct inode *inode = file_inode(file);
> > +		unsigned long flags_mask = file->f_op->mmap_supported_flags;
> > +
> > +		if (!flags_mask)
> > +			flags_mask = LEGACY_MAP_MASK;
> >  
> >  		switch (flags & MAP_TYPE) {
> >  		case MAP_SHARED:
> > +			/*
> > +			 * Silently ignore unsupported flags - MAP_SHARED has
> > +			 * traditionally behaved like that and we don't want
> > +			 * to break compatibility.
> > +			 */
> > +			flags &= flags_mask;
> > +			/*
> > +			 * Force use of MAP_SHARED_VALIDATE with non-legacy
> > +			 * flags. E.g. MAP_SYNC is dangerous to use with
> > +			 * MAP_SHARED as you don't know which consistency model
> > +			 * you will get.
> > +			 */
> > +			flags &= LEGACY_MAP_MASK;
> > +			/* fall through */
> > +		case MAP_SHARED_VALIDATE:
> > +			if (flags & ~flags_mask)
> > +				return -EOPNOTSUPP;
> 
> Hmmm.  I'd expect this to worth more like:
> 
> 		case MAP_SHARED:
> 			/* Ignore all new flags that need validation: */
> 			flags &= LEGACY_MAP_MASK;
> 			/*FALLTHROUGH*/
> 		case MAP_SHARED_VALIDATE:
> 			if (flags & ~file->f_op->mmap_supported_flags)
> 				return -EOPNOTSUPP;
> 
> with the legacy mask always implicitly support as indicated in my
> comment to the XFS patch.

I was thinking about this. Originally I thought that mmap_supported_flags
would allow also to declare some legacy flags as unsupported and also it
seemed as a nicer symmetric interface to me. But I guess the need to mask
out legacy flags is mostly theoretical so I'm fine giving that up. So I'll
change this as you suggest.

> Although even the ignoring in MAP_SHARED seems dangerous, but I guess
> we need that to keep strict backwards compatibility.  In world I'd
> rather do
> 
> 	case MAP_SHARED:
> 		if (flags & ~LEGACY_MAP_MASK)
> 			return -EINVAL;

Yes, I think just ignoring new flags for MAP_SHARED is safer...

								Honza

-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux