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
