Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp"  <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>
To: 'Namjae Jeon' <namjae.jeon@samsung.com>
Cc: "Mori.Takahiro@ab.MitsubishiElectric.co.jp" 
	<Mori.Takahiro@ab.MitsubishiElectric.co.jp>,
	"Motai.Hirotaka@aj.MitsubishiElectric.co.jp" 
	<Motai.Hirotaka@aj.MitsubishiElectric.co.jp>,
	'Sungjong Seo' <sj1557.seo@samsung.com>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2] exfat: integrates dir-entry getting and validation
Date: Mon, 3 Aug 2020 07:31:02 +0000	[thread overview]
Message-ID: <TY2PR01MB287579A95A7994DE2B34E425904D0@TY2PR01MB2875.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <015d01d6663e$1eb8c780$5c2a5680$@samsung.com>

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@dc.MitsubishiElectric.co.jp>

  reply	other threads:[~2020-08-03  7:33 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20200715012304epcas1p23e9f45415afc551beea122e4e1bdb933@epcas1p2.samsung.com>
2020-07-15  1:22 ` Tetsuhiro Kohada
2020-07-29 10:01   ` Tetsuhiro Kohada
2020-07-30  6:53   ` Namjae Jeon
2020-08-03  7:31     ` Kohada.Tetsuhiro [this message]
2020-08-04  1:28       ` Namjae Jeon
2020-08-05  1:44         ` Tetsuhiro Kohada

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=TY2PR01MB287579A95A7994DE2B34E425904D0@TY2PR01MB2875.jpnprd01.prod.outlook.com \
    --to=kohada.tetsuhiro@dc.mitsubishielectric.co.jp \
    --cc=Mori.Takahiro@ab.MitsubishiElectric.co.jp \
    --cc=Motai.Hirotaka@aj.MitsubishiElectric.co.jp \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=namjae.jeon@samsung.com \
    --cc=sj1557.seo@samsung.com \
    --subject='RE: [PATCH v2] exfat: integrates dir-entry getting and validation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).