From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E37F05D912 for ; Fri, 12 Jan 2024 10:10:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="NcLhguKl" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705054258; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=tSQo3iFYuEoV2E2vO3pCVyOF7jXFG5/WTTmBRYm7i4Y=; b=NcLhguKlxbYuKCLmZ2C7NuKGgqtM2f6f5/32WxiUkTifTA3M6NWOfJwUIfQ5GbzgZ57UL5 5y6+rpAcDmog106DCXLlHYr6dwkpt4kf8JJzEUgjwz+Zd+qXufxGIYQ9JGYhk92g5bhCHi n1AkxYWYGmbQtsJaLn5nOgtQLMGtnWQ= Received: from mail-oi1-f199.google.com (mail-oi1-f199.google.com [209.85.167.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-437-JW2lAxmTOB2fEVdZ-N20EQ-1; Fri, 12 Jan 2024 05:10:51 -0500 X-MC-Unique: JW2lAxmTOB2fEVdZ-N20EQ-1 Received: by mail-oi1-f199.google.com with SMTP id 5614622812f47-3bbb6fd2cceso7367650b6e.2 for ; Fri, 12 Jan 2024 02:10:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705054250; x=1705659050; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=tSQo3iFYuEoV2E2vO3pCVyOF7jXFG5/WTTmBRYm7i4Y=; b=tC02bZLhJCJfv41ZT/TtAtwhV8blcLsOWqmyCGUa7IB0RXs6RUuRUIVv+IHrOXR934 Ed1kuGWmUQ080tQqmO7k7grAAB9wtM1K1pMPpFPbPUroPZ/PZQSkix57y6KPhE5mPtAN 74CyN3Y6mFiF5rer2HG4pkNz0gQWwc8VbBpy6r+GLilI3apfp/lunMO3OJX3MdTnGGtq izggaJ0vAbstlmufVXPMNOeBl3F66bDsap2r0os2juBTOu+as1aL83SiLgUaCXnmmxkE qPhonlei0+SYsCZ2FqLGyKWbeFLB8GDmAIOECrIDirbMY1p8WthT2YIHOApuJFXbwtYh zvjg== X-Gm-Message-State: AOJu0YxHkXNs3Q8ejgZ9RftLulv5JZnbsrNf76qwQurHhCEH3QsypXVs FIzGLMBTqFvwQpVmc7aoNSQR/5RK269qg9xeNtQb6sUiWZsdheNfiSsLIVXKsMgeUhGy4oMEbW4 AoB/HnL+AIG0Zv+TSRCagNQ== X-Received: by 2002:a05:6808:f16:b0:3bc:3c51:d5a3 with SMTP id m22-20020a0568080f1600b003bc3c51d5a3mr909878oiw.47.1705054250540; Fri, 12 Jan 2024 02:10:50 -0800 (PST) X-Google-Smtp-Source: AGHT+IHiX+ccOtrtMG5ipO+bwFnTSNjiYH2WUVmxCYEnfve+noLTiiDz+qofGjgqESdd+c2dbG1hbw== X-Received: by 2002:a05:6808:f16:b0:3bc:3c51:d5a3 with SMTP id m22-20020a0568080f1600b003bc3c51d5a3mr909866oiw.47.1705054250141; Fri, 12 Jan 2024 02:10:50 -0800 (PST) Received: from [192.168.1.165] (cpc76484-cwma10-2-0-cust967.7-3.cable.virginm.net. [82.31.203.200]) by smtp.gmail.com with ESMTPSA id m6-20020a62f206000000b006dae568baedsm2825640pfh.24.2024.01.12.02.10.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 12 Jan 2024 02:10:49 -0800 (PST) Message-ID: Date: Fri, 12 Jan 2024 10:10:44 +0000 Precedence: bulk X-Mailing-List: gfs2@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] gfs2: Improve gfs2_consist_inode() usage To: Andreas Gruenbacher Cc: gfs2@lists.linux.dev References: <20240111174258.72123-1-anprice@redhat.com> From: Andrew Price In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 12/01/2024 00:07, Andreas Gruenbacher wrote: > Andy, > > On Thu, Jan 11, 2024 at 6:43 PM Andrew Price wrote: >> gfs2_consist_inode() logs an error message with the source file and line >> number. When we jump before calling it, the line number becomes less >> useful as it no longer relates to the source of the error. To aid >> troubleshooting, replace the gotos with the gfs2_consist_inode() calls >> so that the error messages are more informative. > > this looks acceptable to me. :) > gfs2_consist_inode() is used inconsistently, though: in some cases, it > reports an original error; in other cases, the error has already been > reported and gfs2_consist_inode() only provides additional details. I > guess we can't have nice code. I don't think additional details could hurt. Perhaps there's room for more cleanups around this function though. >> Signed-off-by: Andrew Price >> --- >> fs/gfs2/dir.c | 28 +++++++++++++++++----------- >> fs/gfs2/glops.c | 34 ++++++++++++++++++++-------------- >> fs/gfs2/xattr.c | 28 ++++++++++++++++------------ >> 3 files changed, 53 insertions(+), 37 deletions(-) >> >> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c >> index 560e4624c09f2..47355c87efc3b 100644 >> --- a/fs/gfs2/dir.c >> +++ b/fs/gfs2/dir.c >> @@ -562,15 +562,18 @@ static struct gfs2_dirent *gfs2_dirent_scan(struct inode *inode, void *buf, >> int ret = 0; >> >> ret = gfs2_dirent_offset(GFS2_SB(inode), buf); >> - if (ret < 0) >> - goto consist_inode; >> - >> + if (ret < 0) { >> + gfs2_consist_inode(GFS2_I(inode)); >> + return ERR_PTR(-EIO); >> + } >> offset = ret; >> prev = NULL; >> dent = buf + offset; >> size = be16_to_cpu(dent->de_rec_len); >> - if (gfs2_check_dirent(GFS2_SB(inode), dent, offset, size, len, 1)) >> - goto consist_inode; >> + if (gfs2_check_dirent(GFS2_SB(inode), dent, offset, size, len, 1)) { >> + gfs2_consist_inode(GFS2_I(inode)); >> + return ERR_PTR(-EIO); >> + } >> do { >> ret = scan(dent, name, opaque); >> if (ret) >> @@ -582,8 +585,10 @@ static struct gfs2_dirent *gfs2_dirent_scan(struct inode *inode, void *buf, >> dent = buf + offset; >> size = be16_to_cpu(dent->de_rec_len); >> if (gfs2_check_dirent(GFS2_SB(inode), dent, offset, size, >> - len, 0)) >> - goto consist_inode; >> + len, 0)) { >> + gfs2_consist_inode(GFS2_I(inode)); >> + return ERR_PTR(-EIO); >> + } >> } while(1); >> >> switch(ret) { >> @@ -598,7 +603,6 @@ static struct gfs2_dirent *gfs2_dirent_scan(struct inode *inode, void *buf, >> return ERR_PTR(ret); >> } >> >> -consist_inode: > > The code below is now unreachable, so I'll remove it. Good catch, thanks. Andy > >> gfs2_consist_inode(GFS2_I(inode)); >> return ERR_PTR(-EIO); >> } >> @@ -609,14 +613,16 @@ static int dirent_check_reclen(struct gfs2_inode *dip, >> const void *ptr = d; >> u16 rec_len = be16_to_cpu(d->de_rec_len); >> >> - if (unlikely(rec_len < sizeof(struct gfs2_dirent))) >> - goto broken; >> + if (unlikely(rec_len < sizeof(struct gfs2_dirent))) { >> + gfs2_consist_inode(dip); >> + return -EIO; >> + } >> ptr += rec_len; >> if (ptr < end_p) >> return rec_len; >> if (ptr == end_p) >> return -ENOENT; >> -broken: >> + >> gfs2_consist_inode(dip); >> return -EIO; >> } >> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c >> index 45653cbc8a87d..ae8d4731907d1 100644 >> --- a/fs/gfs2/glops.c >> +++ b/fs/gfs2/glops.c >> @@ -409,10 +409,14 @@ static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf) >> struct inode *inode = &ip->i_inode; >> bool is_new = inode->i_state & I_NEW; >> >> - if (unlikely(ip->i_no_addr != be64_to_cpu(str->di_num.no_addr))) >> - goto corrupt; >> - if (unlikely(!is_new && inode_wrong_type(inode, mode))) >> - goto corrupt; >> + if (unlikely(ip->i_no_addr != be64_to_cpu(str->di_num.no_addr))) { >> + gfs2_consist_inode(ip); >> + return -EIO; >> + } >> + if (unlikely(!is_new && inode_wrong_type(inode, mode))) { >> + gfs2_consist_inode(ip); >> + return -EIO; >> + } >> ip->i_no_formal_ino = be64_to_cpu(str->di_num.no_formal_ino); >> inode->i_mode = mode; >> if (is_new) { >> @@ -449,26 +453,28 @@ static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf) >> /* i_diskflags and i_eattr must be set before gfs2_set_inode_flags() */ >> gfs2_set_inode_flags(inode); >> height = be16_to_cpu(str->di_height); >> - if (unlikely(height > sdp->sd_max_height)) >> - goto corrupt; >> + if (unlikely(height > sdp->sd_max_height)) { >> + gfs2_consist_inode(ip); >> + return -EIO; >> + } >> ip->i_height = (u8)height; >> >> depth = be16_to_cpu(str->di_depth); >> - if (unlikely(depth > GFS2_DIR_MAX_DEPTH)) >> - goto corrupt; >> + if (unlikely(depth > GFS2_DIR_MAX_DEPTH)) { >> + gfs2_consist_inode(ip); >> + return -EIO; >> + } >> ip->i_depth = (u8)depth; >> ip->i_entries = be32_to_cpu(str->di_entries); >> >> - if (gfs2_is_stuffed(ip) && inode->i_size > gfs2_max_stuffed_size(ip)) >> - goto corrupt; >> - >> + if (gfs2_is_stuffed(ip) && inode->i_size > gfs2_max_stuffed_size(ip)) { >> + gfs2_consist_inode(ip); >> + return -EIO; >> + } >> if (S_ISREG(inode->i_mode)) >> gfs2_set_aops(inode); >> >> return 0; >> -corrupt: >> - gfs2_consist_inode(ip); >> - return -EIO; >> } >> >> /** >> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c >> index 8c96ba6230d1b..17ae5070a90e6 100644 >> --- a/fs/gfs2/xattr.c >> +++ b/fs/gfs2/xattr.c >> @@ -96,30 +96,34 @@ static int ea_foreach_i(struct gfs2_inode *ip, struct buffer_head *bh, >> return -EIO; >> >> for (ea = GFS2_EA_BH2FIRST(bh);; prev = ea, ea = GFS2_EA2NEXT(ea)) { >> - if (!GFS2_EA_REC_LEN(ea)) >> - goto fail; >> + if (!GFS2_EA_REC_LEN(ea)) { >> + gfs2_consist_inode(ip); >> + return -EIO; >> + } >> if (!(bh->b_data <= (char *)ea && (char *)GFS2_EA2NEXT(ea) <= >> - bh->b_data + bh->b_size)) >> - goto fail; >> - if (!gfs2_eatype_valid(sdp, ea->ea_type)) >> - goto fail; >> + bh->b_data + bh->b_size)) { >> + gfs2_consist_inode(ip); >> + return -EIO; >> + } >> + if (!gfs2_eatype_valid(sdp, ea->ea_type)) { >> + gfs2_consist_inode(ip); >> + return -EIO; >> + } >> error = ea_call(ip, bh, ea, prev, data); >> if (error) >> return error; >> >> if (GFS2_EA_IS_LAST(ea)) { >> if ((char *)GFS2_EA2NEXT(ea) != >> - bh->b_data + bh->b_size) >> - goto fail; >> + bh->b_data + bh->b_size) { >> + gfs2_consist_inode(ip); >> + return -EIO; >> + } >> break; >> } >> } >> >> return error; >> - >> -fail: >> - gfs2_consist_inode(ip); >> - return -EIO; >> } >> >> static int ea_foreach(struct gfs2_inode *ip, ea_call_t ea_call, void *data) >> -- >> 2.43.0 >> > > Thanks, > Andreas >