On Tue, 07 Aug 2007 08:00:40 +0200 Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > (cutting out lists from CC) > > > > > > Your patch is changing the API in a very unsafe way, since there will > > > > > be no error or warning on an unconverted fs. And that could lead to > > > > > security holes. > > > > > > > > > > If we would rename the setattr method to setattr_new as well as > > > > > changing it's behavior, that would be fine. But I guess we do not > > > > > want to do that. > > > > > > > > Which "unconverted fses"? If we're talking out of tree stuff, then too > > > > bad: it is _their_ responsibility to keep up with kernel changes. > > > > > > It is usually a good idea to not change the semantics of an API in a > > > backward incompatible way without changing the syntax as well. > > > > We're taking two setattr flags ATTR_KILL_SGID, and ATTR_KILL_SUID which > > have existed for several years in the VFS, and making them visible to > > the filesystems. Out-of-tree filesystems that care can check for them in > > a completely backward compatible way: you don't even need to add a > > #define. > > Making flags visible is not the problem. > > Making another flag invisible (ATTR_MODE) at the same time _is_. > > > > This is true regardless of whether we care about out-of-tree code or > > > not (and we should care to some degree). And especially true if the > > > change in question is security sensitive. > > > > It is not true "regardless": the in-tree code is being converted. > > Out-of-tree code is the only "problem" here, and their only problem is > > that they may have to add support for the new flags if they also support > > suid/sgid mode bits. > > Yes. And there would be no problem with that, as long as it would be > breaking the API in a visible way. It does not do that and that is > unsafe. > > The other thing with this change is, that it's generally a good idea > to let the VFS do as much as possible, because then filesystem writers > won't get it wrong. > > In this case the suid/sgid check needs to be omitted for _very_ few > filesystems (NFS and fuse). So it makes sense to leave the check > outside filesystem code, and move it inside only when necessary. > On the other hand, the filesystem writers here are declaring their own setattr operation. Is it unreasonable for them to take responsibility for handling this too? There are other in-tree filesystems that likely need to have this check omitted (other networked filesystems in particular), but this patchset tries to make this change transparent for now. Those authors can go back and fix this up later. As Trond said, in-tree filesystems will be converted so they won't be an issue. The only danger is someone who is running unconverted out-of-tree filesystem code on a kernel with this patch. Is that enough of an issue to warrant us taking extra steps to deal with it? Another alternative might be to rename notify_change(). I don't really like gratuitously breaking the API, though, and that function is referenced in a lot of documentation. Changing it might be confusing... -- Jeff Layton <jlayton@xxxxxxxxxx> - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
