| Message ID | 20140509214056.GC497@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 09, 2014 at 05:40:57PM -0400, J. Bruce Fields wrote: > On Fri, May 09, 2014 at 03:55:03PM +0800, Kinglong Mee wrote: > > On 5/9/2014 00:12, J. Bruce Fields wrote: > > > I'd like to apply this if only to remove the redundant code. > > > > > > I'd like to understand, though, whether this is something that caused an > > > actual practical problem for someone, or if you just happened to notice > > > the inconsistency between nfs and ext4 behavior? > > > > I test it with ext2,ext3,btrfs,xfs. > > Test result is same as ext4. > > So, we needs remove the redundant killing of suid/sgid. > > Understood that this would make the behavior consistent with > filesystems. But, you don't know of any cases of the current behavior > is actually causing a problem for anyone? I thin this also is the root cause for xfstests generic/193 failing on NFS, but I haven't verified it yet. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/10/2014 13:10, Christoph Hellwig wrote: > On Fri, May 09, 2014 at 05:40:57PM -0400, J. Bruce Fields wrote: >> On Fri, May 09, 2014 at 03:55:03PM +0800, Kinglong Mee wrote: >>> On 5/9/2014 00:12, J. Bruce Fields wrote: >>>> I'd like to apply this if only to remove the redundant code. >>>> >>>> I'd like to understand, though, whether this is something that caused an >>>> actual practical problem for someone, or if you just happened to notice >>>> the inconsistency between nfs and ext4 behavior? >>> >>> I test it with ext2,ext3,btrfs,xfs. >>> Test result is same as ext4. >>> So, we needs remove the redundant killing of suid/sgid. >> >> Understood that this would make the behavior consistent with >> filesystems. But, you don't know of any cases of the current behavior >> is actually causing a problem for anyone? > > I thin this also is the root cause for xfstests generic/193 failing on > NFS, but I haven't verified it yet. xfstests generic/193 only tests non-root user truncating file with root setting SGID/SUID mode. generic/193 will not fail. 236 _create_files 237 # Now test out the clear of suid/sgid for truncate 238 # 239 echo "check that suid/sgid bits are cleared after successful truncate..." 240 241 echo "with no exec perm" 242 echo frobnozzle >> $test_user 243 chmod ug+s $test_user 244 echo -n "before: "; stat -c '%A' $test_user 245 su ${qa_user} -c "echo > $test_user" 246 echo -n "after: "; stat -c '%A' $test_user 247 248 echo "with user exec perm" 249 echo frobnozzle >> $test_user 250 chmod ug+s $test_user 251 chmod u+x $test_user 252 echo -n "before: "; stat -c '%A' $test_user 253 su ${qa_user} -c "echo > $test_user" 254 echo -n "after: "; stat -c '%A' $test_user 255 256 echo "with group exec perm" 257 echo frobnozzle >> $test_user 258 chmod ug+s $test_user 259 chmod g+x $test_user 260 chmod u-x $test_user 261 echo -n "before: "; stat -c '%A' $test_user 262 su ${qa_user} -c "echo > $test_user" 263 echo -n "after: "; stat -c '%A' $test_user 264 265 echo "with user+group exec perm" 266 echo frobnozzle >> $test_user 267 chmod ug+s $test_user 268 chmod ug+x $test_user 269 echo -n "before: "; stat -c '%A' $test_user 270 su ${qa_user} -c "echo > $test_user" 271 echo -n "after: "; stat -c '%A' $test_user thanks, Kinglong Mee -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 16, 2014 at 03:31:09PM +0800, Kinglong Mee wrote: > xfstests generic/193 only tests non-root user truncating file > with root setting SGID/SUID mode. generic/193 will not fail. generic/193 reproducibly fails for me. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 16f0673..6aaa305 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -857,20 +857,6 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file, return err; } -static void kill_suid(struct dentry *dentry) -{ - struct iattr ia; - ia.ia_valid = ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV; - - mutex_lock(&dentry->d_inode->i_mutex); - /* - * Note we call this on write, so notify_change will not - * encounter any conflicting delegations: - */ - notify_change(dentry, &ia, NULL); - mutex_unlock(&dentry->d_inode->i_mutex); -} - /* * Gathered writes: If another process is currently writing to the file, * there's a high chance this is another nfsd (triggered by a bulk write @@ -942,10 +928,6 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file, nfsdstats.io_write += host_err; fsnotify_modify(file); - /* clear setuid/setgid flag after write */ - if (inode->i_mode & (S_ISUID | S_ISGID)) - kill_suid(dentry); - if (stable) { if (use_wgather) host_err = wait_for_concurrent_writes(file);