[LU-19004] server_ioctl uses LU_SEQ_RANGE_MDT wrongly

• Type: Bug
• Resolution: Unresolved
• Priority: Minor
• Fix Version/s: Lustre 2.17.0
• Affects Version/s: Lustre 2.17.0
• Labels: easy smatch
• Severity: 3
• Assignee: WC Triage
• Reporter: Oleg Drokin

Description

smatch highlights that lustre/target/tgt_mount.c::server_ioctl() has this code:

               err = obd_get_info(env, lsi->lsi_osd_exp, sizeof(KEY_FID2IDX),
                                  KEY_FID2IDX, &len, &u.fid);
               if (err == 0) {
                       err = -EINVAL;
                       if (u.range.lsr_flags & LU_SEQ_RANGE_MDT)
                               err = u.range.lsr_index;

The problem is 

#define LU_SEQ_RANGE_MDT  0x0

so it never hits (introduced in https://review.whamcloud.com/c/fs/lustre-release/+/56339)

Overall these values are definet as if they are the bits in a bitmask, but are never used as such which makes everything super confusing, so probably ripe for some rework?

#define LU_SEQ_RANGE_MDT        0x0
#define LU_SEQ_RANGE_OST        0x1
#define LU_SEQ_RANGE_ANY        0x3
#define LU_SEQ_RANGE_MASK       0x3

For example:

 static inline unsigned fld_range_type(const struct lu_seq_range *range)
{
        return range->lsr_flags & LU_SEQ_RANGE_MASK;
}

With the saving grace here being that all values are actually then explicitly compared.

 

Issue Links

• is related to: LU-18212 add LL_IOC_FID2MDTIDX ioctl support on server's mount point.
• is related to: LU-2681 need to handle lsr_flags having other flags set

Comments

[Oleg Drokin]

soundslike the only user of the _MASK is fld_range_type

And the only user of fld_range_type outside of fld_range_is_xxx is fld_local_lookup.

I wonder what stops us from dropping fld_range_type completely and replace it with just using hte range value directly in all cases?

[Oleg Drokin]

yes, misuse is just one place, but it's facilitated by very unfortunate and confusing naming of these defines and surrounding functions.

So we have to address that as well or more similar breakage would appear.

E.g. I think the whole "MASK" thing could be removed and all users revisited"

[Andreas Dilger]

Note, it appears that this is the only similar misuse of LU_SEQ_RANGE_MDT as a mask. All other users either set or check equivalence to either LU_SEQ_RANGE_MDT or LU_SEQ_RANGE_OST.

[Andreas Dilger]

Overall these values are definet as if they are the bits in a bitmask, but are never used as such which makes everything super confusing, so probably ripe for some rework?

#define LU_SEQ_RANGE_MDT        0x0
#define LU_SEQ_RANGE_OST        0x1
#define LU_SEQ_RANGE_ANY        0x3
#define LU_SEQ_RANGE_MASK       0x3

These constants are in lustre_idl.h so they form part of the on-disk format and/or network protocol, so they cannot just be changed. It looks like the check should be changed to use the existing helper functions that implement this correctly (which I found after I had proposed an almost identical helper function to implement this check):

                if (fld_range_is_mdt(&u.range))
                        err = u.range.lsr_index;