Date:   Mon, 3 Aug 2020 07:31:02 +0000
From:   "Kohada.Tetsuhiro@...MitsubishiElectric.co.jp" 
        <Kohada.Tetsuhiro@...MitsubishiElectric.co.jp>
To:     'Namjae Jeon' <namjae.jeon@...sung.com>
CC:     "Mori.Takahiro@...MitsubishiElectric.co.jp" 
        <Mori.Takahiro@...MitsubishiElectric.co.jp>,
        "Motai.Hirotaka@...MitsubishiElectric.co.jp" 
        <Motai.Hirotaka@...MitsubishiElectric.co.jp>,
        'Sungjong Seo' <sj1557.seo@...sung.com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] exfat: integrates dir-entry getting and validation

Thank you for your reply.

> > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> > 573659bfbc55..09b85746e760 100644
> > --- a/fs/exfat/dir.c
> > +++ b/fs/exfat/dir.c
> > @@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb,  {
> >  	int i;
> >  	struct exfat_entry_set_cache *es;
> > +	struct exfat_dentry *ep;
> >
> >  	es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> >  	if (!es)
> > @@ -44,13 +45,9 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> >  	 * Third entry  : first file-name entry
> >  	 * So, the index of first file-name dentry should start from 2.
> >  	 */
> > -	for (i = 2; i < es->num_entries; i++) {
> > -		struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> > -
> > -		/* end of name entry */
> > -		if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> > -			break;
> >
> > +	i = 2;
> > +	while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> As Sungjong said, I think that TYPE_NAME seems right to be validated in exfat_get_dentry_set().

First, it is possible to correctly determine that 
"Immediately follow the Stream Extension directory entry as a consecutive series" 
whether the TYPE_NAME check is implemented here or exfat_get_dentry_set().
It's functionally same, so it is also right to validate in either.

Second, the current implementation does not care for NameLength field, as I replied to Sungjong.
If name is not terminated with zero, the name will be incorrect.(With or without my patch)
I think TYPE_NAME and NameLength validation should not be separated from the name extraction.
If validate TYPE_NAME in exfat_get_dentry_set(), NameLength validation and name extraction 
should also be implemented there.
(Otherwise, a duplication check with exfat_get_dentry_set() and here.)
I will add NameLength validation here.
Therefore, TYPE_NAME validation here should not be omitted.

Third, getting dentry and entry-type validation should be integrated.
These no longer have to be primitive.
The integration simplifies caller error checking.


> > -struct exfat_dentry *exfat_get_dentry_cached(
> > -	struct exfat_entry_set_cache *es, int num)
> > +struct exfat_dentry *exfat_get_validated_dentry(struct exfat_entry_set_cache *es,
> > +						int num, unsigned int type)
> Please use two tabs.

OK.
I'll fix it.


> > +	struct buffer_head *bh;
> > +	struct exfat_dentry *ep;
> >
> > -	return (struct exfat_dentry *)p;
> > +	if (num >= es->num_entries)
> > +		return NULL;
> > +
> > +	bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
> > +	if (!bh)
> > +		return NULL;
> > +
> > +	ep = (struct exfat_dentry *)
> > +		(bh->b_data + EXFAT_BLK_OFFSET(off, es->sb));
> > +
> > +	switch (type) {
> > +	case TYPE_ALL: /* accept any */
> > +		break;
> > +	case TYPE_FILE:
> > +		if (ep->type != EXFAT_FILE)
> > +			return NULL;
> > +		break;
> > +	case TYPE_SECONDARY:
> > +		if (!(type & exfat_get_entry_type(ep)))
> > +			return NULL;
> > +		break;
> Type check should be in this order : FILE->STREAM->NAME->{CRITICAL_SEC|BENIGN_SEC}
> I think that you are missing TYPE_NAME check here.

Types other than the above (TYPE_NAME, TYPE_STREAM, etc.) are checked in the default-case(as below).

> > +	default:
> > +		if (type != exfat_get_entry_type(ep))
> > +			return NULL;
> > +	}
> > +	return ep;
> >  }
> >
> >  /*
> >   * Returns a set of dentries for a file or dir.
> >   *
> > - * Note It provides a direct pointer to bh->data via exfat_get_dentry_cached().
> > + * Note It provides a direct pointer to bh->data via exfat_get_validated_dentry().
> >   * User should call exfat_get_dentry_set() after setting 'modified' to apply
> >   * changes made in this entry set to the real device.
> >   *
> >   * in:
> >   *   sb+p_dir+entry: indicates a file/dir
> > - *   type:  specifies how many dentries should be included.
> > + *   max_entries:  specifies how many dentries should be included.
> >   * return:
> >   *   pointer of entry set on success,
> >   *   NULL on failure.
> > + * note:
> > + *   On success, guarantee the correct 'file' and 'stream-ext' dir-entries.
> This comment seems unnecessary.

I'll remove it.

> > diff --git a/fs/exfat/file.c b/fs/exfat/file.c index
> > 6707f3eb09b5..b6b458e6f5e3 100644
> > --- a/fs/exfat/file.c
> > +++ b/fs/exfat/file.c
> > @@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
> >  				ES_ALL_ENTRIES);
> >  		if (!es)
> >  			return -EIO;
> > -		ep = exfat_get_dentry_cached(es, 0);
> > -		ep2 = exfat_get_dentry_cached(es, 1);
> > +		ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> > +		ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
> TYPE_FILE and TYPE_STREAM was already validated in exfat_get_dentry_set().
> Isn't it unnecessary duplication check ?

No, as you say.
Although TYPE is specified, it is not good not to check the null of ep/ep2.
However, with TYPE_ALL, it becomes difficult to understand what purpose ep/ep2 is used for.
Therefore, I proposed adding ep_file/ep_stream to es, and here
	ep = es->ep_file;
	ep2 = es->ep_stream;

How about this?
Or is it better to specify TYPE_ALL?


BTW
It's been about a month since I posted this patch.
In the meantime, I created a NameLength check and a checksum validation based on this patch.
Can you review those as well?

BR
---
Kohada Tetsuhiro <Kohada.Tetsuhiro@...MitsubishiElectric.co.jp>

Powered by blists - more mailing lists