linux-kernel - Re: [PATCHv2] btrfs: Introduce new BTRFS_IOC_SNAP_DESTROY

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] [day] [month] [year] [list]

Message-ID: <20200129161214.GJ3929@twin.jikos.cz>
Date:   Wed, 29 Jan 2020 17:12:14 +0100
From:   David Sterba <dsterba@...e.cz>
To:     Marcos Paulo de Souza <mpdesouza@...e.de>
Cc:     dsterba@...e.cz, linux-kernel@...r.kernel.org, dsterba@...e.com,
        josef@...icpanda.com, linux-btrfs@...r.kernel.org,
        Marcos Paulo de Souza <mpdesouza@...e.com>
Subject: Re: [PATCHv2] btrfs: Introduce new BTRFS_IOC_SNAP_DESTROY_V2 ioctl

On Wed, Jan 29, 2020 at 12:07:40PM -0300, Marcos Paulo de Souza wrote:
> > > -	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
> > > -	namelen = strlen(vol_args->name);
> > > -	if (strchr(vol_args->name, '/') ||
> > > -	    strncmp(vol_args->name, "..", namelen) == 0) {
> > > -		err = -EINVAL;
> > > -		goto out;
> > > +		if (!(vol_args2->flags & BTRFS_SUBVOL_BY_ID)) {
> > > +			err = -EINVAL;
> > > +			goto out;
> > 
> > The flag validation needs to be factored out of the if. First
> > validate,
> > then do the rest. For backward compatibility, the v1 ioctl must take
> > no
> > flags, so if theres BTRFS_SUBVOL_BY_ID for v1, it needs to fail. For
> > v2
> > the flag is optional.
> 
> Only vol_args_v2 has the flags field, so for current
> BTRFS_IOC_SNAP_DESTORY there won't be any flags. If we drop the check
> for BTRFS_SUBVOL_BY_ID in BTRFS_IOC_SNAP_DESTORY_V2, so won't check for
> this flag at all, making it meaningless.

Oh right, so the validation applies only to v2 and in that case it's
fine to keep it in the place you have it.

> What do you think? Should we drop this flag at all and just rely in the
> ioctl number + subvolid being informed?

No, v2 should work for both deletion by name and by id. It's going to
supersede v1 that has to stay for backward compatibility, but must
provide complete functionality of v1 to keep the usability sane.