Re: [PATCH v8 09/12] xen: add runtime parameter access support to hypfs

[Jürgen Groß <jgross@xxxxxxxx>]

On 14.05.20 14:10, Jan Beulich wrote:
> On 14.05.2020 13:48, Jürgen Groß wrote:
> > On 14.05.20 12:20, Jan Beulich wrote:
> > > On 08.05.2020 17:34, Juergen Gross wrote:
> > > > --- a/xen/common/grant_table.c
> > > > +++ b/xen/common/grant_table.c
> > > > @@ -85,8 +85,43 @@ struct grant_table {
> > > >        struct grant_table_arch arch;
> > > >    };
> > > >    -static int parse_gnttab_limit(const char *param, const char *arg,
> > > > -                              unsigned int *valp)
> > > > +unsigned int __read_mostly opt_max_grant_frames = 64;
> > > > +static unsigned int __read_mostly opt_max_maptrack_frames = 1024;
> > > > +
> > > > +#ifdef CONFIG_HYPFS
> > > > +#define GRANT_CUSTOM_VAL_SZ  12
> > > > +static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ];
> > > > +static char __read_mostly opt_max_maptrack_frames_val[GRANT_CUSTOM_VAL_SZ];
> > > > +
> > > > +static void update_gnttab_par(struct param_hypfs *par, unsigned int val,
> > > > +                              char *parval)
> > > > +{
> > > > +    snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%u", val);
> > > > +    custom_runtime_set_var_sz(par, parval, GRANT_CUSTOM_VAL_SZ);
> > > > +}
> > > > +
> > > > +static void __init gnttab_max_frames_init(struct param_hypfs *par)
> > > > +{
> > > > +    update_gnttab_par(par, opt_max_grant_frames, opt_max_grant_frames_val);
> > > > +}
> > > > +
> > > > +static void __init max_maptrack_frames_init(struct param_hypfs *par)
> > > > +{
> > > > +    update_gnttab_par(par, opt_max_maptrack_frames,
> > > > +                      opt_max_maptrack_frames_val);
> > > > +}
> > > > +#else
> > > > +#define opt_max_grant_frames_val    NULL
> > > > +#define opt_max_maptrack_frames_val NULL
> > > This looks latently dangerous to me (in case new uses of these
> > > two identifiers appeared), but I guess my alternative suggestion
> > > will be at best controversial, too:
> > >
> > > #define update_gnttab_par(par, val, unused) update_gnttab_par(par, val)
> > > #define parse_gnttab_limit(par, arg, valp, unused) parse_gnttab_limit(par, arg, 
> > > valp)
> > >
> > > (placed right here)
> > Who else would use those identifiers not related to hypfs?
> I can't see an obvious possible use, but people get creative, i.e.
> you never know. Passing NULL into a function without it being
> blindingly obvious that it won't ever get (de)referenced is an at
> least theoretical risk imo.
Hmm, what about using a special type for those content variables?
Something like:

#ifdef CONFIG_HYPFS
#define hypfs_string_var            char *
#else
#define hypfs_string_var            char
#define opt_max_grant_frames_val    0
#define opt_max_maptrack_frames_val 0
#endif

And then use that as type for function parameters? This should make
dereferencing pretty hard.

Other than that I have no really good idea how to avoid this problem.

> > > > +#define custom_runtime_set_var_sz(parfs, var, sz) \
> > > > +    { \
> > > > +        (parfs)->hypfs.content = var; \
> > > > +        (parfs)->hypfs.e.max_size = sz; \
> > > var and sz want parentheses around them.
> > Fine with me, but out of curiosity: what can go wrong without? Are
> > you thinking of multi-statement arguments?
> Well, as just said in the reply on the patch 4 thread, you have
> a point about this being the right side of an assignment.
> Nevertheless such uses would look more consistent if parenthesized.
> The only cases where I see it reasonable to omit parentheses are
> - parameters made the operands of # or ##,
> - parameters handed on to further macros / functions unaltered,
> - parameters representing struct/union field names,
> - perhaps other special cases along the lines of the above that
>    I can't seem to be able to think of right now.
Okay.

> > > > +/* initfunc needs to set size and content, e.g. via custom_runtime_set_var(). 
> > > > */
> > > > +#define custom_runtime_only_param(_name, _var, initfunc) \
> > > I've started noticing it here, but the issue exists further up
> > > (and down) as well - please can you avoid identifiers with
> > > leading underscores that are in violation of the C standard?
> > > Even more so that here you're not even consistent across
> > > macro parameter names.
> > Basically I only extended the macro to take another parameter and I
> > omitted the underscore exactly for the reason you mentioned.
> >
> > In case you like it better I can prepend a patch to the series dropping
> > all the leading single underscores from the macros in param.h.
> That's code churn I don't view as strictly necessary - adjusting
> these can be done when they get touched anyway. But here we have
> a whole new body of code.
Okay.

> > > > +#define param_2_parfs(par)  NULL
> > > Along the lines of the earlier comment, this looks latently dangerous.
> > In which way? How can the empty struct be dereferenced in a way not
> > resulting in build time errors, other than using a cast which would
> > be obviously wrong for the standard case when CONFIG_HYPFS is defined?
> Have the result of the macro passed to a function taking void *, e.g.
> memcpy().
Any function like this needs either a guaranteed minimum size or a
specific size as parameter. I can't imagine a valid use case leading
to problems, TBH.


Juergen